-
Notifications
You must be signed in to change notification settings - Fork 818
feat(reexecute/c): decouple metrics server and collector #4415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
0178776
feat(reexecution/c): decouple metrics server and collector
RodrigoVillar 4ad3f2b
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar fc83c89
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar 848c6ad
docs: improve collectRegistry
RodrigoVillar be761ac
chore: set default to empty string
RodrigoVillar ca0b993
docs: benchmark script
RodrigoVillar a7cb056
chore: simplify metricsMode
RodrigoVillar d36d4ca
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar c7f3185
chore: unexport metricsMode
RodrigoVillar 9e319d0
chore: clean up
RodrigoVillar 2cc0a79
chore: self-review nits
RodrigoVillar 5160738
chore: split metrics flag
RodrigoVillar 1832b58
chore: switch
RodrigoVillar 915ae5d
chore: reduce duplicate code
RodrigoVillar d846616
Merge branch 'master' into rodrigo/decouple-reexecution-metrics
RodrigoVillar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this just implicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it's own, I'm not opposed to
METRICS_COLLECTOR_ENABLED=trueimplicitly settingMETRICS_SERVER_ENABLED=trueas well. However, considering that this PR will be followed up by #4418 (which adds the ability to configure a port for the metrics server), I think this becomes confusing (i.e. it isn't clear what happens ifMETRICS_COLLECTOR_ENABLED=trueandMETRICS_PORT=Xwithout reading the description ofMETRICS_COLLECTOR_ENABLED).This could be fixed by renaming
METRICS_COLLECTOR_ENABLEDtoMETRICS_SERVER_AND_COLLECTOR_ENABLED, but this looks similar to a previous iteration of this PR which received this review comment: #4415 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to block on this, but a rethink is definitely suggested. I don't think the comment you linked to suggesting seperate flags precludes improving what appears in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? The only options regarding the flag design are as follows:
With this PR choosing the latter, I'm not sure what else needs to be rethought (if you think enabling the collector needs to implicitly enable the server, I'm happy to follow-up on that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is the chosen terminology.
ENABLE_METRICS_SERVERstarts aPrometheusServerinstance, and that name could be confused with an actual Prometheus server (one that collects and aggregate metrics). I recommend changingENABLE_METRICS_SERVERtoENABLE_METRICS_EXPORTandPrometheusServertoMetricsExporter. That way there is a unambiguous relationship between exporting metrics and collecting those exported metrics. Once that change is made, I think it would be a no-brainer to implicitly enable metrics export when collection is enabled rather than requiring that the user explicitly configure that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. Repeating what I said in #4418, enabling the metrics collector in #4418 will implicitly enable the metrics server with a port of
0(as of commit 830e8c1)