feat: Add user fields mapping to OTLP #431

ericywl · 2025-01-24T07:09:56Z

❓ Why is this being changed

Related to elastic/apm-server#15254

🧑‍💻 What is being changed

Map the following user fields from OTel attributes to APMEvent.User

user.id
user.name
user.email

The other user fields do not have equivalents in APMEvent.User, so they will still be under APMEvent.Labels.

✅ How to validate the change

Run test on APM server with the changes

ericywl · 2025-01-24T07:12:01Z

input/otlp/metadata.go

@@ -580,3 +580,19 @@ func replaceReservedRune(disallowedRunes string) func(r rune) rune {
 		return unicode.ToLower(r)
 	}
 }
+
+// NOTE: This will overwrite user fields set by `user.name=process.owner` from `otlp/metadata.go:translateResourceMetadata`.


This requires some consideration, how should we handle it if the user.name was already set by process.owner?

ref:

apm-data/input/otlp/metadata.go

Lines 239 to 243 in 99f84c0

case semconv.AttributeProcessOwner:

if out.User == nil {

out.User = &modelpb.User{}

}

out.User.Name = truncate(v.Str())

See test cases at #431 (comment)

Raised in elastic/apm-server#15254 (comment) that process.owner mapping to user.name seems a bit out of place. I wonder it should actually be mapped to process.user.name instead.

ericywl · 2025-01-24T07:32:55Z

input/otlp/metadata_test.go

+		{
+			name:         "process owner not overwritten",
+			processOwner: "i-am-the-owner",
+			input: userField{
+				userID:    "123",
+				userName:  "",
+				userEmail: "[email protected]",
+			},
+			expected: userField{
+				userID:    "123",
+				userName:  "i-am-the-owner",
+				userEmail: "[email protected]",
+			},
+		},
+		{
+			name:         "overwrite process owner",
+			processOwner: "i-am-the-owner",
+			input: userField{
+				userID:    "123",
+				userName:  "hello",
+				userEmail: "[email protected]",
+			},
+			expected: userField{
+				userID:    "123",
+				userName:  "hello",
+				userEmail: "[email protected]",
+			},
+		},


Here are the test cases for when the process.owner resource metadata attribute is set.

this is the case for record-level (log records, data points, spans) attributes overwriting resource level attributes, which is fine. But you may also need to test resource level User semconv fields competing with process.owner. However, I'm not sure if we should expect it at the resource level. See the above comment.

ericywl · 2025-01-24T10:21:10Z

input/otlp/metadata.go

@@ -242,6 +242,8 @@ func translateResourceMetadata(resource pcommon.Resource, out *modelpb.APMEvent)
 			}
 			out.User.Name = truncate(v.Str())

+		// TODO(eric): Check if `user.*` fields are considered resource metadata?


Need help to confirm if user.* attributes are:

resource metadata only

scope metadata only

datapoints only

resource metadata + datapoints

scope metadata + datapoints

resource metadata + scope metadata + datapoints

I see that you are currently only doing it for log records, data points and span events. imo it is missing for spans (see TranslateTransaction and TranslateSpan) which is most likely where these attributes are.

And you wonder if it should apply to resource and scope. I think it is unlikely for scope, but not too sure about resource. @axw may have a better idea here.

Thanks. From what I can tell, TranslateTransaction and TranslateSpan is also used by intake v2. Will this change overwrite the user fields that are set here?

I see that you are currently only doing it for log records, data points and span events. imo it is missing for spans (see TranslateTransaction and TranslateSpan) which is most likely where these attributes are.

++

And you wonder if it should apply to resource and scope. I think it is unlikely for scope, but not too sure about resource. @axw may have a better idea here.

I'm not aware of any cases where user would exist at the resource level. I guess there could be some kind of resource owner attributes, but I'm only aware of the need to represent users of the instrumented service, which would be at the operation level.

Got it, I will just add the change to TranslateTransaction and TranslateSpan.

Thanks. From what I can tell, TranslateTransaction and TranslateSpan is also used by intake v2. Will this change overwrite the user fields that are set here?

@axw @carsonip Will this part be a concern?

As discussed in #431 (comment) , I think we should really fix user.name=process.owner in either this PR or a separate PR as a blocker. process.owner shouldn't be setting user.name in the first place. When that's fixed, this PR won't be overwriting user fields.

+1 on this suggestion. @ericywl can you put up a PR to changing the current user mapping as a bugfix and then get back to this PR?

carsonip · 2025-01-27T10:25:19Z

input/otlp/metadata.go

@@ -242,6 +242,8 @@ func translateResourceMetadata(resource pcommon.Resource, out *modelpb.APMEvent)
 			}
 			out.User.Name = truncate(v.Str())

+		// TODO(eric): Check if `user.*` fields are considered resource metadata?


I see that you are currently only doing it for log records, data points and span events. imo it is missing for spans (see TranslateTransaction and TranslateSpan) which is most likely where these attributes are.

And you wonder if it should apply to resource and scope. I think it is unlikely for scope, but not too sure about resource. @axw may have a better idea here.

carsonip · 2025-01-27T10:27:49Z

input/otlp/metadata_test.go

+		{
+			name:         "process owner not overwritten",
+			processOwner: "i-am-the-owner",
+			input: userField{
+				userID:    "123",
+				userName:  "",
+				userEmail: "[email protected]",
+			},
+			expected: userField{
+				userID:    "123",
+				userName:  "i-am-the-owner",
+				userEmail: "[email protected]",
+			},
+		},
+		{
+			name:         "overwrite process owner",
+			processOwner: "i-am-the-owner",
+			input: userField{
+				userID:    "123",
+				userName:  "hello",
+				userEmail: "[email protected]",
+			},
+			expected: userField{
+				userID:    "123",
+				userName:  "hello",
+				userEmail: "[email protected]",
+			},
+		},


this is the case for record-level (log records, data points, spans) attributes overwriting resource level attributes, which is fine. But you may also need to test resource level User semconv fields competing with process.owner. However, I'm not sure if we should expect it at the resource level. See the above comment.

input/otlp/metadata.go

carsonip · 2025-01-27T10:29:54Z

input/otlp/traces_test.go

+			}
+
+			allEvents := transformTraces(t, traces)
+			events := (*allEvents)[1:]


Why is the first event ignored? Is that the span itself?

Yes I believe so.

It shouldn't be ignored. We should ensure that the span document has user fields as well.

carsonip

On the point about user fields being experimental, imo it is fine to handle them and map them to the most appropriate ECS fields, as we have been doing that in apm-server. The implication of this PR is that User semconv fields will be mapped differently, which makes it possibly a "breaking change".

For apm-server, if we're only doing that in a major version change, it is fine, but it also means an apm-data major release and any other apm-data changes to apm-server 8.x will require backport to the previous apm-data major.

For MIS, there's likely a different policy for breaking changes.

Therefore, I'd appreciate @simitt's views on whether this should really be considered a breaking change. It seems like a slippery slope to me.

Co-authored-by: Carson Ip <[email protected]>

…TranslateSpan

simitt · 2025-02-03T08:36:58Z

For MIS, there's likely a different policy for breaking changes.

Therefore, I'd appreciate @simitt's views on whether this should really be considered a breaking change. It seems like a slippery slope to me.

Apologies for the late response. Correcting the current mapping of the user.* attributes is a bugfix which we should get in.
For the new mapping of user.* fields, we need alignment with @mlunadia on how to treat otel mappings in APM Server and MIS. Let's put this on hold for some more days until we have figured this out.

simitt · 2025-02-03T10:40:56Z

Let's close this PR, as we have closed the underlying issue as Won't fix. As raised by @lahsivjar , the currently mapped user fields are used in the Observability UIs with the current semantics of process owner. With the focus on otel native, let's not break this behaviour, but rather ensure the mapping is proper for the otel native solution.

Add user fields mapping to otlp

f86e95f

ericywl commented Jan 24, 2025

View reviewed changes

Add TODOs to confirm if metadata should be updated

256abb5

ericywl commented Jan 24, 2025

View reviewed changes

ericywl changed the title ~~Add user fields mapping to otlp~~ feat: Add user fields mapping to otlp Jan 27, 2025

ericywl changed the title ~~feat: Add user fields mapping to otlp~~ feat: Add user fields mapping to OTLP Jan 27, 2025

carsonip reviewed Jan 27, 2025

View reviewed changes

ericywl and others added 3 commits January 31, 2025 09:34

Update input/otlp/metadata.go

6797da8

Co-authored-by: Carson Ip <[email protected]>

Fix updateEvent and add user fields mapping to TranslateTransaction, …

8414dda

…TranslateSpan

Merge branch 'main' into otel-map-user-fields

eba528a

ericywl closed this Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add user fields mapping to OTLP #431

feat: Add user fields mapping to OTLP #431

ericywl commented Jan 24, 2025 •

edited

Loading

ericywl Jan 24, 2025 •

edited

Loading

ericywl Jan 24, 2025 •

edited

Loading

carsonip Jan 27, 2025

ericywl Jan 24, 2025 •

edited

Loading

carsonip Jan 27, 2025

ericywl Jan 24, 2025

carsonip Jan 27, 2025

ericywl Jan 27, 2025

axw Jan 28, 2025

ericywl Jan 31, 2025

ericywl Jan 31, 2025

carsonip Jan 31, 2025

simitt Feb 3, 2025

carsonip Jan 27, 2025

carsonip Jan 27, 2025

carsonip Jan 27, 2025

ericywl Jan 31, 2025

carsonip Jan 31, 2025

carsonip left a comment

simitt commented Feb 3, 2025

simitt commented Feb 3, 2025

	case semconv.AttributeProcessOwner:
	if out.User == nil {
	out.User = &modelpb.User{}
	}
	out.User.Name = truncate(v.Str())

feat: Add user fields mapping to OTLP #431

feat: Add user fields mapping to OTLP #431

Conversation

ericywl commented Jan 24, 2025 • edited Loading

❓ Why is this being changed

🧑‍💻 What is being changed

✅ How to validate the change

ericywl Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

ericywl Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericywl Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonip left a comment

Choose a reason for hiding this comment

simitt commented Feb 3, 2025

simitt commented Feb 3, 2025

ericywl commented Jan 24, 2025 •

edited

Loading

ericywl Jan 24, 2025 •

edited

Loading

ericywl Jan 24, 2025 •

edited

Loading

ericywl Jan 24, 2025 •

edited

Loading