fix(metrics): ostream exporter should print out resource attributes #1523
Conversation
bsarden
force-pushed
the
dev/metrics-resources-ostream-exporter
branch
from
64f6ab5 to
cccc5b4
Compare
bsarden
force-pushed
the
dev/metrics-resources-ostream-exporter
branch
from
6fbe315 to
a5e10f1
Compare
Codecov Report
@@ Coverage Diff @@
## main #1523 +/- ##
==========================================
+ Coverage 83.74% 83.81% +0.08%
==========================================
Files 156 156
Lines 4850 4873 +23
==========================================
+ Hits 4061 4084 +23
Misses 789 789
|
bsarden
force-pushed
the
dev/metrics-resources-ostream-exporter
branch
from
603b7eb to
79124bb
Compare
| @@ -151,6 +156,11 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData) | |||
| "\n counts : [200, 300, 400, 500, ]" | |||
| "\n attributes\t\t: " | |||
| "\n\ta1: b1" | |||
| "\n resources\t:" | |||
| "\n\tservice.name: unknown_service" | |||
| "\n\ttelemetry.sdk.version: 1.5.0" | |||
There was a problem hiding this comment.
Can we use the macro OPENTELEMETRY_SDK_VERSION instead of the hardcoding version? This way, we don't need to modify this part of the code with every new release.
There was a problem hiding this comment.
Good call. Done. There are some strange cmake failures that I'm having difficulties reproducing on my ubuntu 20.04 machine locally, but they are also not marked as "required" on the CI stages...
There was a problem hiding this comment.
@lalitb: What are the next steps I need to take here in order to debug? It's not clear to me what the next debugging steps are (or how some of these CI failures are relevant to a largely NFC patch like this one).
|
@bsarden - I see these two failing tests are running on Windows with OTLP exporter enabled, would it be possible to test your changes on Windows? Else, I can test it sometime this week. |
|
@lalitb Thanks for the quick response! Unfortunately, I don't have access to a windows host for testing ;(. If you could test on your end sometime this week, that would be great. |
| const std::unordered_map<std::string, sdk::common::OwnedAttributeValue> &map, | ||
| const std::string prefix) | ||
| { | ||
| for (const auto &kv : map) |
There was a problem hiding this comment.
@bsarden - Looks like this part of the code is causing problem. As it's unordered_map, the order of printing of resource elements is implementation specific (so different in Windows and Linux). Can we ensure to print them in the given order - maybe move resource data to std::map first and then print, and then also ensure that the same order is used for comparison in unit-test? Or else do some smart comparison for resource data in unit-test.
There was a problem hiding this comment.
@bsarden - Just wondering if you have a plan to fix CI as mentioned above? Else, I can do it on top of your repo if you are fine :)
There was a problem hiding this comment.
Thanks for fixing it. CI looks good now :)
There was a problem hiding this comment.
Thanks for the ping and reproducing the error on a windows box!
While debugging my
Resourcedefinition for a metrics exporter, I realizedthe
OStreamExporterdid not support printing outResourcecontents.I plan on expanding upon the existing unit(s) for checking the resource
attributes, but wanted to "test the waters" first.
My goal was to attach git branch/sha information to metrics that are collected
for easier A/B testing, and a
Resourceseemed like a natural place for that.For example, using the attributes:
However, there seems to be some debate around using resources and metrics
with prometheus, which is what led me down my debugging path of using the ostream exporter first.