Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping #2261

ie-pham · 2023-03-27T14:09:45Z

What this PR does: Add the service label to job in span metrics and add instance as additional intrinsic dimensions. Add feature to allow custom dimension label mapping.

Dimension Mapping config:

span_metrics+: {
          dimensions: [],
          dimension_mappings: [
            { // label would be "vulture-zero" and the value would be "<vulture-0>"
              name: 'vulture-zero',
              source_labels: ['vulture-0'],
              join: '/'
            },
            { // label would be "vulture-index" and the value would be "<vulture-0>/<vulture-1>"
              name: 'vulture-index',
              source_labels: ['vulture-0', 'vulture-1'],
              join: '/'
            },
          ],
        },

Decisions for target_info/job/instance:
#2261 (comment)

If service.name is present, then job is present. If not, the label is skipped. If ONLY service.namespace is present and not service.name, then also job is skipped.

If service.instance.id is present then instance is present. If not, the label is skipped.

If there are NO resource labels other than the job and instance labels, then the target_info is skipped entirely.

target_info should contain all resource attributes + job + instance

Make sure this is a non-breaking change

Which issue(s) this PR fixes:
Fixes #

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

cyrille-leclerc · 2023-03-28T07:25:46Z

@ie-pham thanks for this great news.
I have a question please: do we rename service into job or do we compose job as:

${service.namespace}/${service.name} if service.namespace is defined
${service.name} otherwise?

cc @gouthamve @jpkrohling

mapno

Left a few comments. Please correct me if I'm wrong, I'm no expert in the requirements for target_info.

modules/generator/processor/spanmetrics/spanmetrics.go

modules/generator/processor/util/util.go

modules/generator/processor/spanmetrics/spanmetrics.go

jpkrohling · 2023-03-28T12:40:33Z

Is this change drifting this component further away from the upstream component? Is there an intention of converging them into one?

modules/generator/processor/spanmetrics/spanmetrics_test.go

knylander-grafana · 2023-03-28T16:09:35Z

@ie-pham Do we need documentation for this? It looks like new features that need content.

ie-pham · 2023-03-28T18:30:31Z

@ie-pham Do we need documentation for this? It looks like new features that need content.

yes we do, can you point me to where i need to update this?

gouthamve · 2023-03-28T21:09:35Z

Also this will cause an increase in cardinality because we are not including service.instance.id in the labels. It should be documented clearly.

joe-elliott

So w/o finding every line I've noticed that some of these changes will impact existing cloud users. Specifically the changes will increase cardinality and impact their bill. We either need the defaults to not change the metrics that are being produced or we need to communicate this change in advance in some clear and official way to our cloud users.

Will look to @gouthamve to provide guidance.

jpkrohling

I'm happy with the changes from an OTel perspective, as I understand they will indeed bring this closer to the expected pipelines in upstream (otelcol with span to metrics connector and Prometheus exporter)

modules/generator/processor/spanmetrics/config.go

ie-pham · 2023-03-30T19:39:31Z

@gouthamve @jpkrohling @kovrus can the user disable job and instance?

kovrus · 2023-03-31T07:49:57Z

@gouthamve @jpkrohling @kovrus can the user disable job and instance?

What is the motivation behind that?

Those are generated by Prometheus remote write, it is not possible to disable it manually, but if the service.name or service.instance.id is missing then they won't be created https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/prometheusremotewrite/helper.go#L188-L205. When using span metrics connector with prom remote write there always be service.name generated (if not, those points will be dropped).

modules/generator/processor/spanmetrics/spanmetrics.go

kovrus · 2023-03-31T08:03:47Z

@ie-pham is it possible to add some tests for the target info generation?

docs/sources/tempo/metrics-generator/span_metrics.md

ie-pham · 2023-03-31T14:19:41Z

@ie-pham is it possible to add some tests for the target info generation?

@kovrus tests have already been added to "spanmetrics_test"

zalegrala

This looks good to me. Nice work. A couple comments but otherwise I'm +1 here.

docs/sources/tempo/configuration/_index.md

modules/overrides/limits.go

mapno · 2023-04-04T10:12:31Z

modules/generator/processor/spanmetrics/spanmetrics.go

+	p.spanMetricsCallsTotal.UpdateLabels(p.labels)
+	p.spanMetricsDurationSeconds.UpdateLabels(p.labels)
+	p.spanMetricsSizeTotal.UpdateLabels(p.labels)
+	p.spanMetricsTargetInfo.UpdateLabels(p.targetInfoLabels)


Not sure if this is thread-safe. Batches can be concurrently pushed to a processor. If a set of traces has instance_id, and other doesn't, how does that affect all the labels that are being mutated in the metrics and the processor itself (eg. line 174)?

good catch! i made some changes and added tests for this scenario but yeah I'm not sure how to handle concurrency. Do you have any recommendations on how to handle it?

… and the ability to customize dimension label mapping (grafana#2261) * add job, instance, target_info * add test & gofmt * fmt & test * fix e2e test * add custom dimension mapping * updated test for custom mapping and changelog * fmt * changed the dimension mapping logic * fix test * update logic for job instance * fix test & lint * add service back and add gauge for target_info * added overrides and docs * undo config test change * typo * change traces_spanmetrics_target_info to just target_info * lint * fix overrides * handle batches with different resource attributes * redo counter * refactor registry * refactor label pair * oops * oops * rebase mishap * remove job instance by default * refactor * rebase hell * count * lint * remove arbitary number * traces_target_info * comments * sanitize labels for target info * small refactor to improve perf * rebased * rebased

…2447) * Add config param for autocomplete filtering (#2433) * Add config param for autocomplete filtering * Docs * Fix * Fix test * Address comments * Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping (#2261) * add job, instance, target_info * add test & gofmt * fmt & test * fix e2e test * add custom dimension mapping * updated test for custom mapping and changelog * fmt * changed the dimension mapping logic * fix test * update logic for job instance * fix test & lint * add service back and add gauge for target_info * added overrides and docs * undo config test change * typo * change traces_spanmetrics_target_info to just target_info * lint * fix overrides * handle batches with different resource attributes * redo counter * refactor registry * refactor label pair * oops * oops * rebase mishap * remove job instance by default * refactor * rebase hell * count * lint * remove arbitary number * traces_target_info * comments * sanitize labels for target info * small refactor to improve perf * rebased * rebased --------- Co-authored-by: Jennie Pham <[email protected]>

ie-pham changed the title ~~add job, instance, target_info~~ Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping Mar 28, 2023

ie-pham marked this pull request as ready for review March 28, 2023 01:48

ie-pham requested review from joe-elliott, annanay25, mdisibio, mapno, yvrhdn and zalegrala as code owners March 28, 2023 01:48

gouthamve requested review from jpkrohling and kovrus March 28, 2023 07:37

mapno reviewed Mar 28, 2023

View reviewed changes

zalegrala reviewed Mar 28, 2023

View reviewed changes

modules/generator/processor/spanmetrics/spanmetrics_test.go Outdated Show resolved Hide resolved

joe-elliott reviewed Mar 29, 2023

View reviewed changes

jpkrohling approved these changes Mar 29, 2023

View reviewed changes

joe-elliott reviewed Mar 30, 2023

View reviewed changes

modules/generator/processor/spanmetrics/config.go Outdated Show resolved Hide resolved

ie-pham requested a review from knylander-grafana as a code owner March 31, 2023 01:47

kovrus reviewed Mar 31, 2023

View reviewed changes

modules/generator/processor/spanmetrics/spanmetrics.go Outdated Show resolved Hide resolved

kovrus reviewed Mar 31, 2023

View reviewed changes

modules/generator/processor/spanmetrics/spanmetrics.go Outdated Show resolved Hide resolved

gouthamve reviewed Mar 31, 2023

View reviewed changes

docs/sources/tempo/metrics-generator/span_metrics.md Outdated Show resolved Hide resolved

zalegrala reviewed Mar 31, 2023

View reviewed changes

docs/sources/tempo/configuration/_index.md Show resolved Hide resolved

modules/overrides/limits.go Outdated Show resolved Hide resolved

mapno reviewed Apr 4, 2023

View reviewed changes

ie-pham added 26 commits May 8, 2023 16:33

add service back and add gauge for target_info

0decdff

added overrides and docs

c25d50a

undo config test change

ca9be56

typo

e777266

change traces_spanmetrics_target_info to just target_info

dd44fd2

lint

1844d90

fix overrides

8af8e8c

handle batches with different resource attributes

a795b14

redo counter

9e9a2a7

refactor registry

97dcae7

refactor label pair

8d7722b

oops

05a1bcb

oops

8b2574f

rebase mishap

81a6773

remove job instance by default

a2f4b8c

refactor

45d875d

rebase hell

8cee996

count

2b3d3b2

lint

98acf95

remove arbitary number

da1bef9

traces_target_info

7697d0a

comments

27e8c62

sanitize labels for target info

07dcad9

small refactor to improve perf

8cdaaea

rebased

b8daba7

rebased

2a245ec

ie-pham merged commit 2981488 into grafana:main May 9, 2023

mapno mentioned this pull request May 10, 2023

[r95] patch with autocomplete_filtering flag and target_info PRs #2447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping #2261

Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping #2261

ie-pham commented Mar 27, 2023 •

edited

Loading

cyrille-leclerc commented Mar 28, 2023 •

edited

Loading

mapno left a comment

jpkrohling commented Mar 28, 2023

knylander-grafana commented Mar 28, 2023

ie-pham commented Mar 28, 2023

gouthamve commented Mar 28, 2023

joe-elliott left a comment

jpkrohling left a comment •

edited

Loading

ie-pham commented Mar 30, 2023

kovrus commented Mar 31, 2023

kovrus commented Mar 31, 2023

ie-pham commented Mar 31, 2023 •

edited

Loading

zalegrala left a comment

mapno Apr 4, 2023

ie-pham Apr 5, 2023 •

edited

Loading

Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping #2261

Add job & instance labels to span metrics, a new target_info metrics, and the ability to customize dimension label mapping #2261

Conversation

ie-pham commented Mar 27, 2023 • edited Loading

cyrille-leclerc commented Mar 28, 2023 • edited Loading

mapno left a comment

Choose a reason for hiding this comment

jpkrohling commented Mar 28, 2023

knylander-grafana commented Mar 28, 2023

ie-pham commented Mar 28, 2023

gouthamve commented Mar 28, 2023

joe-elliott left a comment

Choose a reason for hiding this comment

jpkrohling left a comment • edited Loading

Choose a reason for hiding this comment

ie-pham commented Mar 30, 2023

kovrus commented Mar 31, 2023

kovrus commented Mar 31, 2023

ie-pham commented Mar 31, 2023 • edited Loading

zalegrala left a comment

Choose a reason for hiding this comment

mapno Apr 4, 2023

Choose a reason for hiding this comment

ie-pham Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

ie-pham commented Mar 27, 2023 •

edited

Loading

cyrille-leclerc commented Mar 28, 2023 •

edited

Loading

jpkrohling left a comment •

edited

Loading

ie-pham commented Mar 31, 2023 •

edited

Loading

ie-pham Apr 5, 2023 •

edited

Loading