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

Call scopeInfoMetrics only once even if it returns an error #4499

Merged
merged 7 commits into from Sep 14, 2023
Merged

Call scopeInfoMetrics only once even if it returns an error #4499

merged 7 commits into from Sep 14, 2023

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented Sep 12, 2023 •
edited

Closes #4091.

@dmathieu dmathieu force-pushed the no-double-call-scopeinfometrics branch from dd03f27 to f87115c Compare September 12, 2023 14:03
@codecov
Copy link

codecov bot commented Sep 12, 2023 •
edited

Codecov Report

Merging #4499 (b316d42) into main (e3d6c8e) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4499   +/-   ##
=====================================
  Coverage   81.3%   81.3%           
=====================================
  Files        220     220           
  Lines      17670   17678    +8     
=====================================
+ Hits       14378   14386    +8     
  Misses      2992    2992           
  Partials     300     300
Files Changed Coverage
exporters/prometheus/exporter.go 100.0%

pellared
pellared reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Just two comments. Besides that it is great. Especially that you have even covered it in a unit test 👍

exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
@pellared pellared merged commit ac56391 into open-telemetry:main Sep 14, 2023
22 checks passed
@dmathieu dmathieu deleted the no-double-call-scopeinfometrics branch September 14, 2023 07:53
@dmathieu dmathieu mentioned this pull request Sep 14, 2023
Merged
@MrAlias MrAlias mentioned this pull request Sep 14, 2023
Merged
@MrAlias MrAlias mentioned this pull request Sep 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@MrAlias MrAlias MrAlias approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

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

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

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

createScopeInfoMetric is unnecessarily called when it returns an error
3 participants
@dmathieu @pellared @MrAlias