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.

Sign up for GitHub

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

Jump to bottom

Fix prometheus name duplicate _total suffix #3369

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Fix prometheus name duplicate _total suffix #3369

merged 1 commit into from
Oct 21, 2022

Conversation

tony612
Copy link
Contributor

@tony612 tony612 commented Oct 20, 2022 •
edited
Loading

Signed-off-by: Bing Han [email protected]

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

XSAM
XSAM approved these changes Oct 20, 2022
View reviewed changes
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 •
edited
Loading

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%) ⬇️

dmathieu
dmathieu approved these changes Oct 20, 2022
View reviewed changes
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.

hanyuancheung
hanyuancheung approved these changes Oct 20, 2022
View reviewed changes
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

Aneurysm9
Aneurysm9 approved these changes Oct 20, 2022
View reviewed changes
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
@MrAlias MrAlias merged commit ccbc38e into open-telemetry:main Oct 21, 2022
@GeorgeMac
Copy link

GeorgeMac commented Nov 21, 2022 •
edited
Loading

👋 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
@fatsheep9146 fatsheep9146 mentioned this pull request Nov 22, 2022
Closed
@jmacd jmacd mentioned this pull request Dec 2, 2022
Closed
malud pushed a commit to coupergateway/couper that referenced this pull request Dec 5, 2022
additional fixups; rm total suffix
bd63ba9 
still broken due to open-telemetry/opentelemetry-go#3369
@malud malud mentioned this pull request Dec 5, 2022
Merged
@MadVikingGod MadVikingGod mentioned this pull request Dec 5, 2022
Merged
malud pushed a commit to coupergateway/couper that referenced this pull request Dec 16, 2022
additional fixups; rm total suffix
c1b1dd9 
still broken due to open-telemetry/opentelemetry-go#3369
malud pushed a commit to coupergateway/couper that referenced this pull request Dec 30, 2022
Pkg updates (#621)
e66e509 
* 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
@MrAlias MrAlias mentioned this pull request Jan 30, 2023
Closed
@gouthamve gouthamve mentioned this pull request Jul 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanyuancheung hanyuancheung hanyuancheung approved these changes

@XSAM XSAM XSAM approved these changes

@MrAlias MrAlias MrAlias approved these changes

@dashpole dashpole dashpole approved these changes

@dmathieu dmathieu dmathieu approved these changes

@jmacd jmacd Awaiting requested review from jmacd

@evantorrie evantorrie Awaiting requested review from evantorrie

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@pellared pellared Awaiting requested review from pellared pellared is a code owner

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

Assignees
No one assigned
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
Go: Metric SDK (Beta)
Status: Done
Milestone
Metric v0.34.0
Development

Successfully merging this pull request may close these issues.
8 participants
@tony612 @GeorgeMac @MrAlias @dmathieu @Aneurysm9 @dashpole @XSAM @hanyuancheung