Skip to content

Modify KvToString() Method in Instrument.h to Allow Valid Label Names in Records For Prometheus#298

Merged
reyang merged 5 commits into
open-telemetry:masterfrom
erichsueh3:instrument_fix
Aug 29, 2020
Merged

Modify KvToString() Method in Instrument.h to Allow Valid Label Names in Records For Prometheus#298
reyang merged 5 commits into
open-telemetry:masterfrom
erichsueh3:instrument_fix

Conversation

@erichsueh3
Copy link
Copy Markdown
Contributor

This PR is in reference to issue #297 and changes the KvToString() method in Instrument.h. Additionally, there are changes to the print_value() method because KvToString() calls this method for translating a label’s value. Finally, the boolean jsonTypes parameter in print_value() is left unused; this PR does not remove it, though it does not see use anywhere else and has been left in as a precautionary measure.

cc @alolita, @reyang, @pyohannes

… allows for Prometheus to Collect() on records correctly
@erichsueh3 erichsueh3 requested a review from a team August 25, 2020 23:03
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 26, 2020

Codecov Report

Merging #298 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
- Coverage   94.60%   94.59%   -0.01%     
==========================================
  Files         146      146              
  Lines        6633     6629       -4     
==========================================
- Hits         6275     6271       -4     
  Misses        358      358              
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/metrics/instrument.h 93.84% <100.00%> (-0.36%) ⬇️
sdk/test/metrics/metric_instrument_test.cc 100.00% <100.00%> (ø)

@erichsueh3
Copy link
Copy Markdown
Contributor Author

The Bazel on MacOS test seems to be failing due to a missing dependency in zpages; not sure if this is related to this PR or not.

@erichsueh3
Copy link
Copy Markdown
Contributor Author

@ankit-bhargava @Brandon-Kimberly Do you two have any insight on why a double quote character is needed in a label name? I'm not sure if I'm missing something in the specification or if there's another reason for this

@Brandon-Kimberly
Copy link
Copy Markdown
Contributor

@ankit-bhargava @Brandon-Kimberly Do you two have any insight on why a double quote character is needed in a label name? I'm not sure if I'm missing something in the specification or if there's another reason for this

Ankit implemented the function but as far as I am aware there is no reason for it to be there. I imagine it was just added because it makes it look nicer when exported to stdout. But again, I'm not sure if there is a reason for it.

@reyang reyang merged commit 78012e1 into open-telemetry:master Aug 29, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
update the INSTALL doc to referece the trace target instead of the in…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants