-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: Add support for all Prometheus metric types + internal refactor. #80
Conversation
0485fc3
to
fcda049
Compare
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.
Thanks a lot for doing this refactor and adding all types! Looks really neat! ❤️
Some small comments.
cfg := metrics.NewConfigFromFlags(kingpin.Flag) | ||
port := kingpin.Flag("port", "Port to serve at").Default("9001").Int() | ||
remoteURL := kingpin.Flag("remote-url", "URL to send samples via remote_write API.").URL() | ||
// TODO(bwplotka): Kill pprof feature, you can install OSS continuous profiling easily instead. |
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 agree!
Delete(labels prometheus.Labels) bool | ||
} | ||
|
||
func deleteValues[T seriesDeleter](metrics []T, labelKeys, labelValues []string, seriesCount, seriesCycle int) { |
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.
Nice use of generics!
Fixes #77 Signed-off-by: bwplotka <[email protected]>
Co-authored-by: Saswata Mukherjee <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]>
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.
LGTM!
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.
Big PR to review without much involvement in the project hehe
I've tried it by running and manually checking the /metrics endpoint, LGTM! This looks perfect for benchmarking our work with @Maniktherana in prometheus/prometheus. And if we implement PRWv2 it also looks perfect for the work we're doing with @jmichalek132 in the collector :)
Is there anything in particular I should review here? I don't think I will find time for a full review anytime soon. I don't even know this project. |
Yea don't worry - I didn't know this codebase yesterday either. But anyway, I am actually no longer sure why I pinged you, sorry for spam and considering 🙈 Since no objections I will allow myself to merge this, we can always revert. I pinged current maintainers and not sure if they are still active in this project, will try more and perhaps propose myself in the meantime here. |
prometheus-community#80) * feat: Add support for all Prometheus metric types + internal refactor. Fixes prometheus-community#77 * Apply suggestions from code review Co-authored-by: Saswata Mukherjee <[email protected]> --------- Co-authored-by: Saswata Mukherjee <[email protected]>
This PR refreshes avalanche binary, while adding support for all Prometheus metric type.
This should not break compatibility (even if we can), but I deprecate one flag.
Fixes #77
High level changes (as in the CHANGLOG):
Refactor: