[exporter/tanzuobservability] Don't roll resource tags into metrics #8338

keep94 · 2022-03-08T00:56:20Z

[exporter/tanzuobservability] Don't roll resource tags into metrics

Use the resourcetotelemetry helper instead.

This involves several steps:
1. attributesToTags() works like it did before. It simply concatenates multiple
pdata.AttributeMap values into a single map[string]string
2. Add newAttributeMap() to convert a map[string]string into a
pdata.AttributeMap. This way you can still concatenate a map[string]string
and a pdata.AttributeMap together into a single map[string]string because
the traces exporter still needs to do this.
3. Add getSource() function to get just the source from resource attributes.
4. Add replaceSourceTag() function which replaces "source" with "_source"
in the tags. This way the traces exporter can still work like it did before.
5. Remove the Tags field from the metricInfo type so that it just has the
metric and its source. We don't need to store the resource tags with metrics
anymore.
6. Update all attributesToTags() calls in the metrics exporter so that they
get just the metric tags.
7. Fix all metric exporter tests so that they no longer expect resource
attributes in the metrics.
8. Remove the tests that check that tags names "source" get renamed
to "_source"
9. Use the resourcetotelemetry helper.

Description:
Undo the rolling of resource tags into metrics. In the future we will rely on the resourcetotelemetry helper to do this for us.

Testing:
Unit tests. Run OTEL collector with resourcetotelemetry helper turned off (default). Run OTEL collector with resourcetotelemetry helper turned on.

keep94 · 2022-03-10T00:10:25Z

Hello, build-and-test unit test is failing, but I don't think the failure is related to the changes I made in this PR. I think the errors below are coming from: prometheusremotewriteexporter, but this PR is for the exporter/tanzuobservabilityexporter. Maybe it is a flaky test?

=== RUN   Test_PushMetrics/WAL/staleNaNDoubleGauge_case
=== PAUSE Test_PushMetrics/WAL/staleNaNDoubleGauge_case
=== RUN   Test_PushMetrics/WAL/staleNaNIntSum_case
=== PAUSE Test_PushMetrics/WAL/staleNaNIntSum_case
=== RUN   Test_PushMetrics/WAL/staleNaNSum_case
=== PAUSE Test_PushMetrics/WAL/staleNaNSum_case
=== RUN   Test_PushMetrics/WAL/staleNaNHistogram_case
=== PAUSE Test_PushMetrics/WAL/staleNaNHistogram_case
=== RUN   Test_PushMetrics/WAL/staleNaNEmptyHistogram_case
=== PAUSE Test_PushMetrics/WAL/staleNaNEmptyHistogram_case
=== RUN   Test_PushMetrics/WAL/staleNaNSummary_case
=== PAUSE Test_PushMetrics/WAL/staleNaNSummary_case
=== CONT  Test_PushMetrics/WAL/doubleSum_case
=== CONT  Test_PushMetrics/WAL/emptyCumulativeSum_case
=== CONT  Test_PushMetrics/WAL/staleNaNSummary_case
=== CONT  Test_PushMetrics/WAL/staleNaNHistogram_case
=== CONT  Test_PushMetrics/WAL/staleNaNEmptyHistogram_case
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xd1fd6c]

deosu

Travis, the code changes look good to me. The only comment I can add here is that we are missing unit testing. Are there any in-direct tests we have, which are covering the code changes?

thepeterstone

Looks good - a few minor comments. Thank you!

CHANGELOG.md

exporter/tanzuobservabilityexporter/transformer.go

keep94 · 2022-03-15T21:39:48Z

CHANGELOG.md changes very frequently, so I am constantly rebasing with main to resolve conflicts. Is it possible to review this this PR even though this copy of CHANGELOG.md may have conflicts?

keep94 · 2022-03-15T21:43:19Z

My build was all green before, but with my latest rebase to main, a load test is failing. The only files with conflicts during my rebase were CHANGELOG.md and go.sum. Could this test failure be unrelated to the changes made in this PR?

oppegard

Left one comment, but we'll pair-program on the full review and submit the changes.

oppegard · 2022-03-18T16:33:26Z

exporter/tanzuobservabilityexporter/metrics_test.go

@@ -268,7 +270,7 @@ func TestGaugeConsumerMissingValue(t *testing.T) {
 }

 func createMetricInfo(metric pdata.Metric) metricInfo {
-	mi := metricInfo{Metric: metric, Source: "test_source"}
+	mi := metricInfo{Metric: metric, Source: "test_source", SourceKey: "host.name"}


I think we should inline this helper so that it's easy to see the hard-coded string values that are later used as assertions in the test.

exporter/tanzuobservabilityexporter/metrics_test.go

oppegard

Looks good – thanks Travis!

keep94 · 2022-03-22T16:52:48Z

Hello. My change has been approved internally, but I see that the load tests are failing. These changes to our tanzuobservability exporter involve changing our unit tests. Can unit tests such as our unit tests for the tanzuobservability exporter affect the load tests? It seems that the load tests are failing because too much RAM is being used.

keep94 · 2022-03-22T16:54:34Z

Also, it seems that go.sum go.mod and changelog.md files change often. Is it ok if I rebase once and for all after I have your approval?

keep94 · 2022-03-29T15:19:49Z

It looks like testing the deb package is failing, but I am not sure why. What is the best way to troubleshoot? It looks like the offending command is docker rm -fv otelcontribcol-deb-test >/dev/null 2>&1 || true

jpkrohling · 2022-03-30T15:16:22Z

Can you rebase and fix the conflicts? I think the deb issue might also go away.

keep94 · 2022-03-30T23:36:35Z

@jpkrohling I rebased my PR to the latest version of main to resolve the conflicts. The debian file built, but now the load tests are failing. Could these failures be flaky?

Use the resourcetotelemetry helper instead. This involves several steps: 1. attributesToTags() works like it did before. It simply concatenates multiple pdata.AttributeMap values into a single map[string]string 2. Add newAttributeMap() to convert a map[string]string into a pdata.AttributeMap. This way you can still concatenate a map[string]string and a pdata.AttributeMap together into a single map[string]string because the traces exporter still needs to do this. 3. Add getSource() function to get just the source from resource attributes. 4. Add replaceSourceTag() function which replaces "source" with "_source" in the tags. This way the traces exporter can still work like it did before. 5. Remove the Tags field from the metricInfo type so that it just has the metric and its source. We don't need to store the resource tags with metrics anymore. 6. Update all attributesToTags() calls in the metrics exporter so that they get just the metric tags. 7. Fix all metric exporter tests so that they no longer expect resource attributes in the metrics. 8. Remove the tests that check that tags names "source" get renamed to "_source" 9. Use the resourcetotelemetry helper.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling · 2022-03-31T12:01:23Z

Yes, those are intermittent failures. I rebased this to account for v0.48.0 changes.

keep94 · 2022-03-31T15:00:30Z

Thank you @jpkrohling

keep94 · 2022-03-31T16:24:09Z

@jpkrohling Can this PR be merged?

jpkrohling · 2022-04-01T09:26:51Z

Looks like it has been affected by a flaky test. I just restarted the failed jobs, will merge on green.

keep94 requested a review from a team as a code owner March 8, 2022 00:56

keep94 requested a review from anuraaga March 8, 2022 00:56

project-bot bot added this to In progress in Collector Mar 8, 2022

github-actions bot assigned jpkrohling Mar 8, 2022

keep94 force-pushed the resourcetotelemetry branch 5 times, most recently from dea7d22 to dc92dec Compare March 9, 2022 16:23

deosu reviewed Mar 10, 2022

View reviewed changes

keep94 force-pushed the resourcetotelemetry branch from dc92dec to 71d8626 Compare March 10, 2022 17:09

keep94 mentioned this pull request Mar 10, 2022

[exporter/tanzuobservability] Add metrics to README. #7948

Closed

thepeterstone reviewed Mar 10, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

exporter/tanzuobservabilityexporter/transformer.go Outdated Show resolved Hide resolved

exporter/tanzuobservabilityexporter/transformer.go Show resolved Hide resolved

thepeterstone approved these changes Mar 11, 2022

View reviewed changes

keep94 force-pushed the resourcetotelemetry branch from 781d63d to 346993f Compare March 15, 2022 15:55

keep94 force-pushed the resourcetotelemetry branch 2 times, most recently from c18ed08 to 7f0486e Compare March 17, 2022 16:07

oppegard suggested changes Mar 18, 2022

View reviewed changes

Collector automation moved this from In progress to Review in progress Mar 18, 2022

keep94 force-pushed the resourcetotelemetry branch 2 times, most recently from a9fd93d to 3e5e916 Compare March 21, 2022 16:33

oppegard reviewed Mar 21, 2022

View reviewed changes

exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved

oppegard approved these changes Mar 21, 2022

View reviewed changes

keep94 force-pushed the resourcetotelemetry branch 2 times, most recently from ce969a4 to 5ce27df Compare March 28, 2022 23:41

keep94 force-pushed the resourcetotelemetry branch from 5ce27df to 324b316 Compare March 30, 2022 22:39

keep94 and others added 2 commits March 31, 2022 14:00

Move changelog entry to unreleased

b4eb53b

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling force-pushed the resourcetotelemetry branch from 324b316 to b4eb53b Compare March 31, 2022 12:01

Merge branch 'main' into resourcetotelemetry

b5d201f

jpkrohling approved these changes Apr 5, 2022

View reviewed changes

Collector automation moved this from Review in progress to Reviewer approved Apr 5, 2022

jpkrohling merged commit 517e9c4 into open-telemetry:main Apr 5, 2022

ahayworth mentioned this pull request May 16, 2022

Update to otel-contrib-col v0.51.0 Shopify/opentelemetry-collector-contrib#796

Closed

oppegard mentioned this pull request Apr 12, 2023

REQUEST: New membership for oppegard open-telemetry/community#1421

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/tanzuobservability] Don't roll resource tags into metrics #8338

[exporter/tanzuobservability] Don't roll resource tags into metrics #8338

keep94 commented Mar 8, 2022 •

edited

keep94 commented Mar 10, 2022 •

edited

deosu left a comment

thepeterstone left a comment

keep94 commented Mar 15, 2022

keep94 commented Mar 15, 2022

oppegard left a comment

oppegard Mar 18, 2022

oppegard left a comment

keep94 commented Mar 22, 2022

keep94 commented Mar 22, 2022

keep94 commented Mar 29, 2022 •

edited

jpkrohling commented Mar 30, 2022

keep94 commented Mar 30, 2022

jpkrohling commented Mar 31, 2022

keep94 commented Mar 31, 2022

keep94 commented Mar 31, 2022

jpkrohling commented Apr 1, 2022

[exporter/tanzuobservability] Don't roll resource tags into metrics #8338

[exporter/tanzuobservability] Don't roll resource tags into metrics #8338

Conversation

keep94 commented Mar 8, 2022 • edited

keep94 commented Mar 10, 2022 • edited

deosu left a comment

Choose a reason for hiding this comment

thepeterstone left a comment

Choose a reason for hiding this comment

keep94 commented Mar 15, 2022

keep94 commented Mar 15, 2022

oppegard left a comment

Choose a reason for hiding this comment

oppegard Mar 18, 2022

Choose a reason for hiding this comment

oppegard left a comment

Choose a reason for hiding this comment

keep94 commented Mar 22, 2022

keep94 commented Mar 22, 2022

keep94 commented Mar 29, 2022 • edited

jpkrohling commented Mar 30, 2022

keep94 commented Mar 30, 2022

jpkrohling commented Mar 31, 2022

keep94 commented Mar 31, 2022

keep94 commented Mar 31, 2022

jpkrohling commented Apr 1, 2022

keep94 commented Mar 8, 2022 •

edited

keep94 commented Mar 10, 2022 •

edited

keep94 commented Mar 29, 2022 •

edited