Skip to content
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

Fix prometheus name duplicate _total suffix #3369

Merged
merged 1 commit into from Oct 21, 2022

Conversation

tony612
Copy link
Contributor

@tony612 tony612 commented Oct 20, 2022

Signed-off-by: Bing Han h.bing612@gmail.com

The bug was imported by #3360

Without this fix, the test will fail (foo_milliseconds_total_total ):

    --- FAIL: TestPrometheusExporter/counter (0.00s)
        exporter_test.go:274:
            	Error Trace:	/Users/hanbing/repo/go/opentelemetry-go/exporters/prometheus/exporter_test.go:274
            	Error:      	Received unexpected error:


            	            	Diff:
            	            	--- metric output does not match expectation; want
            	            	+++ got:
            	            	@@ -3,3 +3,5 @@
            	            	 foo_milliseconds_total{A="B",C="D",E="true",F="42"} 24.3
            	            	-foo_milliseconds_total{A="D",C="B",E="true",F="42"} 5
            	            	+# HELP foo_milliseconds_total_total a simple counter
            	            	+# TYPE foo_milliseconds_total_total counter
            	            	+foo_milliseconds_total_total{A="D",C="B",E="true",F="42"} 5
            	            	 # HELP target_info Target metadata
            	Test:       	TestPrometheusExporter/counter

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #3369 (5279caf) into main (1133977) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3369     +/-   ##
=======================================
- Coverage   77.9%   77.9%   -0.1%     
=======================================
  Files        164     164             
  Lines      11323   11323             
=======================================
- Hits        8826    8824      -2     
- Misses      2299    2301      +2     
  Partials     198     198             
Impacted Files Coverage Δ
exporters/prometheus/exporter.go 81.1% <100.0%> (ø)
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also related: #3360, since the regression seems to actually have been introduced there.

Copy link
Member

@hanyuancheung hanyuancheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias added bug Something isn't working area:metrics Part of OpenTelemetry Metrics pkg:exporter:prometheus Related to the Prometheus exporter package labels Oct 20, 2022
@MrAlias MrAlias added this to the Metric v0.34.0 milestone Oct 20, 2022
Signed-off-by: Bing Han <h.bing612@gmail.com>
@MrAlias MrAlias merged commit ccbc38e into open-telemetry:main Oct 21, 2022
@GeorgeMac
Copy link

GeorgeMac commented Nov 21, 2022

👋 any chance this fix can be back-ported onto a v0.33.1 release?

This is playing havoc with my metrics.

(I've moved to this merged commit above meanwhile - so not really blocked).

@MrAlias
Copy link
Contributor

MrAlias commented Nov 21, 2022

wave any chance this fix can be back-ported onto a v0.33.1 release?

This is playing havoc with my metrics.

(I've moved to this merged commit above meanwhile - so not really blocked).

I expect the v0.34.0 release to land early next week. Hopefully that helps.

@GeorgeMac
Copy link

Thanks @MrAlias 🙏

@tony612 tony612 deleted the fix-prom-suffix branch November 22, 2022 04:43
malud pushed a commit to coupergateway/couper that referenced this pull request Dec 5, 2022
malud pushed a commit to coupergateway/couper that referenced this pull request Dec 16, 2022
malud pushed a commit to coupergateway/couper that referenced this pull request Dec 30, 2022
* update vendors

* adopt interface changes from opentelementry upgrade

* upgrade openapi

* use new otel interfaces

* Fix prom exporter setup

* Fixup missing prom labels

* Update backend.go

simplified count calls

* Using sync counters

* additional fixups; rm total suffix

still broken due to open-telemetry/opentelemetry-go#3369

* Update to latest otel version

* test fixup; increase time

* fixup rebase

* log /w unhealthy err type

* filter client error metric for access type

* naming

* request metrics endpoint seems to be more stable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:exporter:prometheus Related to the Prometheus exporter package
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants