-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Migrate Prometheus to the new MetricsExporter interface #1477
Conversation
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1477 +/- ##
=======================================
Coverage 90.92% 90.92%
=======================================
Files 239 239
Lines 16706 16709 +3
=======================================
+ Hits 15190 15193 +3
Misses 1085 1085
Partials 431 431
Continue to review full report at Codecov.
|
for _, metric := range merged { | ||
_ = pe.exporter.ExportMetric(ctx, md.Node, md.Resource, metric) | ||
func (pe *prometheusExporter) ConsumeMetrics(ctx context.Context, md pdata.Metrics) error { | ||
ocmds := pdatautil.MetricsToMetricsData(md) |
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.
Out of curiousity: what's the long term plan for this? Having to convert the data as the very first step in the "new" metric exporter is pretty confusing.
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.
The long term is to change to use the new data directly without oc as intermediate step. Right now focus on changing everything to use the new interfaces so at least we are not everywhere in this transition state and service and config code can be changed to only work with the new interfaces
merge(merged, metric) | ||
} | ||
for _, metric := range merged { | ||
_ = pe.exporter.ExportMetric(ctx, ocmd.Node, ocmd.Resource, metric) |
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.
Unrelated to this PR (the old code is the same), but we shouldn't ignore errors.
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.
You commented before to not do unrelated changes :)
…ry#1477) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
…etry#1477) Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.35.0 to 1.35.2. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.35.0...v1.35.2) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
No description provided.