feat(promexporter): add pagination to dfk [SLT-332] #3292
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
3f6dc6e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2fc5eb36.sanguine-fe.pages.dev |
| Branch Preview URL: | https://dfk-pagination.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3292 +/- ##
====================================================
- Coverage 91.48936% 43.66137% -47.82800%
====================================================
Files 59 74 +15
Lines 1269 2327 +1058
Branches 158 82 -76
====================================================
- Hits 1161 1016 -145
- Misses 104 1305 +1201
- Partials 4 6 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- contrib/promexporter/exporters/dfk.go (1 hunks)
- contrib/promexporter/internal/gql/dfk/client.gen.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/promexporter/exporters/dfk.go
[warning] 30-41: contrib/promexporter/exporters/dfk.go#L30-L41
Added lines #L30 - L41 were not covered by tests
[warning] 43-43: contrib/promexporter/exporters/dfk.go#L43
Added line #L43 was not covered by tests
[warning] 46-46: contrib/promexporter/exporters/dfk.go#L46
Added line #L46 was not covered by testscontrib/promexporter/internal/gql/dfk/client.gen.go
[warning] 62-62: contrib/promexporter/internal/gql/dfk/client.gen.go#L62
Added line #L62 was not covered by tests
[warning] 65-65: contrib/promexporter/internal/gql/dfk/client.gen.go#L65
Added line #L65 was not covered by tests
| func (c *Client) StuckHeroes(ctx context.Context, skip *int64, limit *int64, owner *string, httpRequestOptions ...client.HTTPRequestOption) (*StuckHeroes, error) { | ||
| vars := map[string]interface{}{ | ||
| "skip": skip, | ||
| "limit": limit, |
There was a problem hiding this comment.
Added lines not covered by tests
The static analysis tool indicates that the added lines are not covered by tests. This lack of test coverage may lead to undetected issues in the future.
Consider adding or updating tests to cover the new limit parameter in the StuckHeroes method. This will ensure that the pagination functionality works as expected.
Would you like assistance in generating test cases for the updated StuckHeroes method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: contrib/promexporter/internal/gql/dfk/client.gen.go#L62
Added line #L62 was not covered by tests
[warning] 65-65: contrib/promexporter/internal/gql/dfk/client.gen.go#L65
Added line #L65 was not covered by tests
💡 Codebase verification
Inconsistency between method signature and GraphQL query
The limit parameter has been added to the method signature and the vars map, but the GraphQL query string (StuckHeroesDocument) doesn't use this parameter. This inconsistency may lead to unexpected behavior.
Recommendation:
Update the GraphQL query to include the limit parameter:
const StuckHeroesDocument = `query StuckHeroes ($skip: Int, $owner: String, $limit: Int) {
- heroes(skip: $skip, where: {owner:$owner}) {
+ heroes(skip: $skip, first: $limit, where: {owner:$owner}) {
id
}
}
`Note: Since this is generated code, you may need to update the source from which the code is generated to ensure consistency across the codebase.
🔗 Analysis chain
Inconsistency between method signature and GraphQL query
The limit parameter has been added to the method signature and the vars map, but the GraphQL query string (StuckHeroesDocument) doesn't use this parameter. This inconsistency may lead to unexpected behavior.
Consider updating the GraphQL query to include the limit parameter:
const StuckHeroesDocument = `query StuckHeroes ($skip: Int, $owner: String) {
- heroes(skip: $skip, where: {owner:$owner}) {
+ heroes(skip: $skip, first: $limit, where: {owner:$owner}) {
id
}
}
`Note: Since this is generated code, you may need to update the source of the generation instead.
To confirm if this change is necessary, please run the following command to check if there are any other occurrences of the StuckHeroes query in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of the StuckHeroes query
rg --type go 'query StuckHeroes'
Length of output: 164
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: contrib/promexporter/internal/gql/dfk/client.gen.go#L62
Added line #L62 was not covered by tests
[warning] 65-65: contrib/promexporter/internal/gql/dfk/client.gen.go#L65
Added line #L65 was not covered by tests
| var totalStuckHeroes int64 | ||
| var skip int64 | ||
| var limit int64 = 100 | ||
|
|
||
| for { | ||
| stuckHeroes, err := dfkClient.StuckHeroes(ctx, core.PtrTo(skip), core.PtrTo(limit), core.PtrTo(owner.String())) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get stuck hero count: %w", err) | ||
| } | ||
| totalStuckHeroes += int64(len(stuckHeroes.Heroes)) | ||
| if len(stuckHeroes.Heroes) < int(limit) { | ||
| break | ||
| } | ||
| skip += limit |
There was a problem hiding this comment.
Add unit tests for the new pagination logic
The newly introduced pagination logic in the stuckHeroCountStats function is critical for accurately accumulating the total number of stuck heroes. Currently, the code from lines 30 to 43 is not covered by tests, as indicated by the static analysis tools. To ensure the reliability and correctness of this functionality, please add unit tests that cover various scenarios, such as:
- When the total number of stuck heroes is less than the
limit. - When the total number of stuck heroes is exactly a multiple of the
limit. - When the total number of stuck heroes exceeds the
limit.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-41: contrib/promexporter/exporters/dfk.go#L30-L41
Added lines #L30 - L41 were not covered by tests
[warning] 43-43: contrib/promexporter/exporters/dfk.go#L43
Added line #L43 was not covered by tests
| } | ||
|
|
||
| e.otelRecorder.RecordStuckHeroCount(int64(len(stuckHeroes.Heroes)), chainName) | ||
| e.otelRecorder.RecordStuckHeroCount(totalStuckHeroes, chainName) |
There was a problem hiding this comment.
Ensure metric recording is tested
The call to e.otelRecorder.RecordStuckHeroCount(totalStuckHeroes, chainName) on line 46 is essential for recording the total count of stuck heroes. However, this line is not currently covered by tests, as per the static analysis report. Please add tests to verify that the metric recording functions correctly with the accumulated totalStuckHeroes value.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: contrib/promexporter/exporters/dfk.go#L46
Added line #L46 was not covered by tests
trajan0x
changed the title
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Bug Fixes