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

prevent conflict metric description #3469

Conversation

fatsheep9146
Copy link
Contributor

@fatsheep9146 fatsheep9146 commented Nov 15, 2022

@fatsheep9146 fatsheep9146 force-pushed the prevent-conflict-metric-description branch from a1a652a to 4cb3b8f Compare November 21, 2022 04:51
@fatsheep9146 fatsheep9146 marked this pull request as ready for review November 21, 2022 04:52
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #3469 (cb2627a) into main (e97704c) will increase coverage by 0.0%.
The diff coverage is 83.8%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3469   +/-   ##
=====================================
  Coverage   78.0%   78.1%           
=====================================
  Files        165     165           
  Lines      11755   11809   +54     
=====================================
+ Hits        9179    9228   +49     
- Misses      2380    2385    +5     
  Partials     196     196           
Impacted Files Coverage Δ
exporters/prometheus/exporter.go 83.4% <83.8%> (+0.9%) ⬆️
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+0.8%) ⬆️

@fatsheep9146
Copy link
Contributor Author

@MrAlias @dashpole the pr is ready to be reviews, could you help review that?

@fatsheep9146 fatsheep9146 force-pushed the prevent-conflict-metric-description branch 2 times, most recently from df1967a to 3c84770 Compare November 25, 2022 05:36
@fatsheep9146
Copy link
Contributor Author

@dashpole @MrAlias all comments are fixed, could you help review this again?

@fatsheep9146 fatsheep9146 force-pushed the prevent-conflict-metric-description branch from 97f5e40 to efd6eb0 Compare November 25, 2022 23:43
@MrAlias MrAlias added this to the Metric v0.34.0 milestone Nov 29, 2022
@fatsheep9146 fatsheep9146 force-pushed the prevent-conflict-metric-description branch from a4d8c05 to c68bee6 Compare December 1, 2022 04:51
@fatsheep9146 fatsheep9146 requested a review from MrAlias December 1, 2022 05:10
@fatsheep9146 fatsheep9146 force-pushed the prevent-conflict-metric-description branch from 69f5348 to 7c5e3d8 Compare December 2, 2022 14:45
@fatsheep9146
Copy link
Contributor Author

I add more test cases to cover all possible conflict cases,

  • conflict help for 3 different metric type Counter, UpDownCounter and Histogram
  • conflict unit for 3 different metric type Counter, UpDownCounter and Histogram
  • conflict type for 2 different combinations
    • histogram and updowncounter
    • counter and updowncounter

@Aneurysm9 @dashpole @MrAlias

@MadVikingGod MadVikingGod merged commit 2243431 into open-telemetry:main Dec 5, 2022
codeboten added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Nov 18, 2024
#36356)


#### Description
Fixes bug where exporting fails due to different help messages for the
same metric. With this solution, the exporter will always export metrics
of the same name with the first description it receives. This also
rejects metrics whose types have changed. These changes follow the
[spec](https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#metric-metadata-1):
>Exporters MUST drop entire metrics to prevent conflicting TYPE
comments, but SHOULD NOT drop metric points as a result of conflicting
UNIT or HELP comments. Instead, all but one of the conflicting UNIT and
HELP comments (but not metric points) SHOULD be dropped. If dropping a
comment or metric points, the exporter SHOULD warn the user through
error logging.

Based on open-telemetry/opentelemetry-go#3469

#### Link to tracking issue
Fixes
#28617

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit test cases added.

---------

Co-authored-by: David Ashpole <dashpole@google.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
RutvikS-crest pushed a commit to RutvikS-crest/opentelemetry-collector-contrib that referenced this pull request Dec 9, 2024
open-telemetry#36356)


#### Description
Fixes bug where exporting fails due to different help messages for the
same metric. With this solution, the exporter will always export metrics
of the same name with the first description it receives. This also
rejects metrics whose types have changed. These changes follow the
[spec](https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#metric-metadata-1):
>Exporters MUST drop entire metrics to prevent conflicting TYPE
comments, but SHOULD NOT drop metric points as a result of conflicting
UNIT or HELP comments. Instead, all but one of the conflicting UNIT and
HELP comments (but not metric points) SHOULD be dropped. If dropping a
comment or metric points, the exporter SHOULD warn the user through
error logging.

Based on open-telemetry/opentelemetry-go#3469

#### Link to tracking issue
Fixes
open-telemetry#28617

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit test cases added.

---------

Co-authored-by: David Ashpole <dashpole@google.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
open-telemetry#36356)


#### Description
Fixes bug where exporting fails due to different help messages for the
same metric. With this solution, the exporter will always export metrics
of the same name with the first description it receives. This also
rejects metrics whose types have changed. These changes follow the
[spec](https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#metric-metadata-1):
>Exporters MUST drop entire metrics to prevent conflicting TYPE
comments, but SHOULD NOT drop metric points as a result of conflicting
UNIT or HELP comments. Instead, all but one of the conflicting UNIT and
HELP comments (but not metric points) SHOULD be dropped. If dropping a
comment or metric points, the exporter SHOULD warn the user through
error logging.

Based on open-telemetry/opentelemetry-go#3469

#### Link to tracking issue
Fixes
open-telemetry#28617

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit test cases added.

---------

Co-authored-by: David Ashpole <dashpole@google.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Impossible to create Instrument with the same name from the different MeterProvider
6 participants