[receiver/cloudfoundry]: move datapoint level attributes to resource level #34905

odubajDT · 2024-08-28T12:10:48Z

Description:
Introduce a feature gate enable copying envelope tags to the metrics as resource attributes instead of datapoint attributes.

Link to tracking Issue: #34824

crobert-1

Just a high level review this round, I'll review again once naming is a bit more sorted out 👍

receiver/cloudfoundryreceiver/converter.go

receiver/cloudfoundryreceiver/README.md

receiver/cloudfoundryreceiver/converter.go

crobert-1 · 2024-08-30T18:45:55Z

receiver/cloudfoundryreceiver/receiver.go

 		for _, envelope := range envelopes {
 			if envelope != nil {
 				// There is no concept of startTime in CF loggregator, and we do not know the uptime of the component
 				// from which the metric originates, so just provide receiver start time as metric start time
-				convertEnvelopeToMetrics(envelope, libraryMetrics, cfr.receiverStartTime)
+				convertEnvelopeToMetrics(envelope, metrics.ResourceMetrics().AppendEmpty(), cfr.receiverStartTime)


To be able to conform to OTel metrics formatting with resource attributes, we need to try to create more of a hierarchy here with resource metrics, rather than a flat layout as we have without resource attributes here.

If two metrics share the same resource attributes, they should be in the same resource metric. However, the way the logic currently is written, each metric is in its own resource metric, regardless of whether or not it shares identical resource attributes with other metrics.

I could see two main ways of doing this:

When converting envelopes to metrics, check if a resource metric already exists with the parsed resource attributes. If yes, insert the new metric into the existing resource metric.

After all envelopes have been converted to metrics, deduplicate the resource metrics to merge them.

I think option 1. may be more straight forward and clear to follow the logic of, but I'd welcome more input here.

good point, will adapt using the 1. option

adapted to the new logic, please have a look if you have time. Still adding some more tests, but not done with those yet, so please keep that i mind for now

crobert-1 · 2024-08-30T19:17:30Z

receiver/cloudfoundryreceiver/converter.go

 	namePrefix := envelope.Tags["origin"] + "."

+	if allowResourceAttributes.IsEnabled() {


As @jriguera pointed out in the original issue, not all of the attributes can be considered resource attributes. We should have a predefined list of resource attributes that are copied when the feature gate is enabled.

When the feature gate is enabled the predefined attributes are copied to resource attributes. The rest of the attributes will be datapoint attributes as they are today. The predefined list will only be resource attributes, they will not be both resource attributes and datapoint attributes.

-> org.cloudfoundry.index: Str(4b406f26-f1ec-4d66-8b92-f09ab6105d46) -> org.cloudfoundry.ip: Str(10.0.4.8) -> org.cloudfoundry.job: Str(compute) -> org.cloudfoundry.instance_group: Str(compute) -> org.cloudfoundry.origin: Str(system_metrics_agent) -> org.cloudfoundry.source_id: Str(system_metrics_agent)

@jriguera From your comment on the original issue it sounded like you don't believe the above attributes should be considered resource attributes. Is that correct? If so, can you expand your reasoning? My understanding is that since they describe the resource the metrics are coming from they can be considered resource attributes. Source

ok so the list above is the full list of resource attributes, is that correct?

AFAIK, this should be the full list of resource attributes, please correct me if I am missing something

-> org.cloudfoundry.index -> org.cloudfoundry.ip -> org.cloudfoundry.deployment -> org.cloudfoundry.id -> org.cloudfoundry.job -> org.cloudfoundry.product -> org.cloudfoundry.instance_group -> org.cloudfoundry.instance_id -> org.cloudfoundry.origin -> org.cloudfoundry.system_domain -> org.cloudfoundry.source_id

adapted the logic, here is the list (for now) of resource attributes

odubajDT · 2024-09-13T05:37:42Z

@crobert-1 would you have time to take another look?

crobert-1 · 2024-09-16T16:13:23Z

@crobert-1 would you have time to take another look?

I was hoping to hear back from @jriguera to try and get a consensus around which attributes should be moved to resource attributes here. If we don't hear back soon we can move this forward. 👍

receiver/cloudfoundryreceiver/go.mod

receiver/cloudfoundryreceiver/converter.go

jriguera · 2024-09-23T23:20:04Z

I will continue with my reviewing tomorrow

odubajDT · 2024-09-30T08:16:27Z

I will continue with my reviewing tomorrow

Hi @jriguera, any other comments?

odubajDT · 2024-10-04T05:15:51Z

cc @jriguera

odubajDT · 2024-10-17T06:02:32Z

cc @jriguera

receiver/cloudfoundryreceiver/receiver_test.go

jriguera

Apart of the issues with names of some tests and the flags, it looks good to me.

If I have enough time next days, I would like to test it in my environment.

odubajDT · 2024-10-24T10:02:42Z

@crobert-1 any objections against merging this?

jriguera · 2024-10-24T15:48:30Z

@odubajDT I think @crobert-1 is not available for now, we need to find other person to review and approve the merge

jriguera · 2024-10-24T15:50:01Z

After this is merged, I also will need to apply the same functionality to traces (another PR as a draft which I need to finish)

receiver/cloudfoundryreceiver/converter.go

dmitryax

The code LGTM

…level Signed-off-by: odubajDT <[email protected]>

Signed-off-by: odubajDT <[email protected]>

dmitryax · 2024-12-20T18:01:55Z

.chloggen/move-attributes.yaml

+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: breaking


Is it breaking, given that the feature gate is in alpha? Feel free to submit a PR changing it to enhancement if you agree.

github-actions bot added the receiver/cloudfoundry label Aug 28, 2024

github-actions bot requested a review from crobert-1 August 28, 2024 12:11

odubajDT mentioned this pull request Aug 28, 2024

[receiver/cloudfoundry] Some metric attributes need to be moved to be resource attributes #34824

Open

odubajDT force-pushed the move-attributes branch from 33dd51e to f5a7a6e Compare August 29, 2024 08:47

odubajDT changed the title ~~[receiver/cloudfoundry]: duplicate datapoint attributes to resource~~ [receiver/cloudfoundry]: move datapoint level attributes to resource level Aug 29, 2024

odubajDT marked this pull request as ready for review August 29, 2024 09:36

odubajDT requested a review from a team August 29, 2024 09:36

github-actions bot assigned atoulme Aug 29, 2024

crobert-1 reviewed Aug 29, 2024

View reviewed changes

odubajDT requested a review from crobert-1 August 30, 2024 05:52

crobert-1 reviewed Aug 30, 2024

View reviewed changes

odubajDT marked this pull request as draft September 2, 2024 10:53

odubajDT force-pushed the move-attributes branch 2 times, most recently from 6cfb0b8 to 827da54 Compare September 3, 2024 09:45

odubajDT marked this pull request as ready for review September 3, 2024 11:48

github-actions bot assigned mwear Sep 3, 2024

odubajDT force-pushed the move-attributes branch 2 times, most recently from 8c6601c to 6f34911 Compare September 10, 2024 13:16

github-actions bot added the cmd/otelcontribcol otelcontribcol command label Sep 10, 2024

odubajDT force-pushed the move-attributes branch from c5c9421 to a675f65 Compare September 13, 2024 05:36

odubajDT force-pushed the move-attributes branch from 42b4e5e to 36cb58f Compare September 16, 2024 06:30

odubajDT commented Sep 18, 2024

View reviewed changes

receiver/cloudfoundryreceiver/go.mod Outdated Show resolved Hide resolved

odubajDT commented Sep 18, 2024

View reviewed changes

receiver/cloudfoundryreceiver/go.mod Outdated Show resolved Hide resolved

odubajDT force-pushed the move-attributes branch from c1d06cc to 7466806 Compare September 23, 2024 06:31

odubajDT requested a review from a team as a code owner September 23, 2024 06:31

odubajDT requested a review from fatsheep9146 September 23, 2024 06:31

jriguera reviewed Sep 23, 2024

View reviewed changes

receiver/cloudfoundryreceiver/converter.go Outdated Show resolved Hide resolved

odubajDT force-pushed the move-attributes branch from 27a3988 to 4b8a984 Compare September 30, 2024 08:25

odubajDT force-pushed the move-attributes branch from a0016ff to 54c4e50 Compare October 9, 2024 05:58

jriguera reviewed Oct 17, 2024

View reviewed changes

receiver/cloudfoundryreceiver/receiver_test.go Outdated Show resolved Hide resolved

jriguera reviewed Oct 17, 2024

View reviewed changes

receiver/cloudfoundryreceiver/receiver_test.go Outdated Show resolved Hide resolved

jriguera suggested changes Oct 17, 2024

View reviewed changes

odubajDT force-pushed the move-attributes branch from 9e4df9b to 49a5d0b Compare October 18, 2024 05:44

jriguera approved these changes Oct 22, 2024

View reviewed changes

odubajDT force-pushed the move-attributes branch from 49a5d0b to 0f706bf Compare October 24, 2024 10:02

odubajDT force-pushed the move-attributes branch from ec66156 to dae3634 Compare November 7, 2024 12:05

odubajDT force-pushed the move-attributes branch from 18324ca to b1bdfb2 Compare November 20, 2024 06:24

odubajDT force-pushed the move-attributes branch 2 times, most recently from 8d75948 to 5cb6ed4 Compare December 4, 2024 13:35

dmitryax reviewed Dec 19, 2024

View reviewed changes

receiver/cloudfoundryreceiver/converter.go Outdated Show resolved Hide resolved

dmitryax reviewed Dec 19, 2024

View reviewed changes

odubajDT added 4 commits December 20, 2024 07:05

[receiver/cloudfoundry]: move datapoint level attributes to resource …

9d7f6f2

…level Signed-off-by: odubajDT <[email protected]>

rename function

190321b

Signed-off-by: odubajDT <[email protected]>

polish test cases

f3090b0

Signed-off-by: odubajDT <[email protected]>

adapt versions

0569f6b

Signed-off-by: odubajDT <[email protected]>

odubajDT force-pushed the move-attributes branch from e9af734 to 0569f6b Compare December 20, 2024 06:09

dmitryax approved these changes Dec 20, 2024

View reviewed changes

dmitryax reviewed Dec 20, 2024

View reviewed changes

dmitryax merged commit 96368fa into open-telemetry:main Dec 20, 2024
161 checks passed

github-actions bot added this to the next release milestone Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/cloudfoundry]: move datapoint level attributes to resource level #34905

[receiver/cloudfoundry]: move datapoint level attributes to resource level #34905

odubajDT commented Aug 28, 2024 •

edited

Loading

crobert-1 left a comment

crobert-1 Aug 30, 2024

odubajDT Sep 2, 2024

odubajDT Sep 2, 2024

crobert-1 Aug 30, 2024

odubajDT Sep 2, 2024

odubajDT Sep 2, 2024 •

edited

Loading

odubajDT Sep 2, 2024

odubajDT commented Sep 13, 2024

crobert-1 commented Sep 16, 2024

jriguera commented Sep 23, 2024

odubajDT commented Sep 30, 2024

odubajDT commented Oct 4, 2024

odubajDT commented Oct 17, 2024

jriguera left a comment

odubajDT commented Oct 24, 2024

jriguera commented Oct 24, 2024

jriguera commented Oct 24, 2024

dmitryax left a comment •

edited

Loading

dmitryax Dec 20, 2024

		namePrefix := envelope.Tags["origin"] + "."

		if allowResourceAttributes.IsEnabled() {

[receiver/cloudfoundry]: move datapoint level attributes to resource level #34905

[receiver/cloudfoundry]: move datapoint level attributes to resource level #34905

Conversation

odubajDT commented Aug 28, 2024 • edited Loading

crobert-1 left a comment

Choose a reason for hiding this comment

crobert-1 Aug 30, 2024

Choose a reason for hiding this comment

odubajDT Sep 2, 2024

Choose a reason for hiding this comment

odubajDT Sep 2, 2024

Choose a reason for hiding this comment

crobert-1 Aug 30, 2024

Choose a reason for hiding this comment

odubajDT Sep 2, 2024

Choose a reason for hiding this comment

odubajDT Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

odubajDT Sep 2, 2024

Choose a reason for hiding this comment

odubajDT commented Sep 13, 2024

crobert-1 commented Sep 16, 2024

jriguera commented Sep 23, 2024

odubajDT commented Sep 30, 2024

odubajDT commented Oct 4, 2024

odubajDT commented Oct 17, 2024

jriguera left a comment

Choose a reason for hiding this comment

odubajDT commented Oct 24, 2024

jriguera commented Oct 24, 2024

jriguera commented Oct 24, 2024

dmitryax left a comment • edited Loading

Choose a reason for hiding this comment

dmitryax Dec 20, 2024

Choose a reason for hiding this comment

odubajDT commented Aug 28, 2024 •

edited

Loading

odubajDT Sep 2, 2024 •

edited

Loading

dmitryax left a comment •

edited

Loading