[refactor][storage] Remove metrics factory distinctions from storage factories #6526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1
commented
Jan 11, 2025
plugin/storage/factory.go
Outdated
Comment on lines
141
to
144
| role := "primary" | ||
| if _, ok := factory.(storage.ArchiveFactory); ok { | ||
| role = "archive" | ||
| } |
There was a problem hiding this comment.
@yurishkuro I know this isn't correct but just put it here to start the conversation. This will currently set the role archive whenever the factory implements the ArchiveFactory. Is there a way to know at this point whether the storage is actually being used as archive since this is where the metrics factory is passed to the storage factory.
There was a problem hiding this comment.
Now that I think more about it, the v1 factory will initialize the CLI flags for both the primary namespace and the archive namespace. Since the factories today hold both the primary and archive configs, we probably need to do one of the following:
- Each storage factory should only hold 1 config - This would be ideal but it would require creating two storage meta-factories which I think we decided we don't want.
- Have the meta-factory pass all the metrics factories in with role=primary but override the role in
CreateArchiveSpanReaderto archive.
Let me know what you think
There was a problem hiding this comment.
Went with option 2 for now - if this looks good, we can make the same change for the other storage factories as well
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6526 +/- ##
==========================================
- Coverage 96.27% 96.27% -0.01%
==========================================
Files 372 372
Lines 21283 21278 -5
==========================================
- Hits 20491 20486 -5
Misses 605 605
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1
changed the title
mahadzaryab1
changed the title
ekefan
pushed a commit
to ekefan/jaeger
that referenced
this pull request
Jan 14, 2025
…factories (jaegertracing#6526) ## Which problem is this PR solving? - Towards jaegertracing#6065 ## Description of the changes - Some of the storage factories were currently adding the `role` tag to the metrics factory that it was getting to differentiate between `primary` and `archive`. Given that goal in v2 is not have that distinction, the adding of the `primary` tag was moved to the v1 meta-factory. This tag can then be overriden by the archive specific methods in the storage factories (mainly `CreateArchiveSpanReader` and `CreateArchiveSpanWriter`) with `role=archive`. ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
roletag to the metrics factory that it was getting to differentiate betweenprimaryandarchive. Given that goal in v2 is not have that distinction, the adding of theprimarytag was moved to the v1 meta-factory. This tag can then be overriden by the archive specific methods in the storage factories (mainlyCreateArchiveSpanReaderandCreateArchiveSpanWriter) withrole=archive.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test