-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[pkg/ottl] Add function ProfileID() #39587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[pkg/ottl] Add function ProfileID() #39587
Conversation
bf30889
to
9a6c758
Compare
pkg/ottl/ottlfuncs/README.md
Outdated
|
||
Examples: | ||
|
||
- `ProfileID(0x00000000000000000000000000000000)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A profile_id with all zeroes is considered invalid. Should the example be updated to show a valid profile_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, why is this field "required" (at least one bit must be set)?
I'd say that a receiving backend service can operate without a profile_id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a valid ProfileID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't allowing empty profiles be useful for verifying if some profile_id
(pprofile.ProfileID
) field is empty? It seems the empty rule also applies to trace and span IDs, but their converters are not restricting creating empty values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to construct uses cases supporting both options. But I'd say that's a decision of the profiling SIG. If there are real-world use-cases that need an empty profile_id
, this needs to be discussed in the SIG and then clarified in the protobuf definition and docs.
span_id
is optional: https://github.com/open-telemetry/opentelemetry-proto/blob/f24da8deeb50118271c9435972791ef05ec003b1/opentelemetry/proto/logs/v1/logs.proto#L205
trace_id
is optional: https://github.com/open-telemetry/opentelemetry-proto/blob/f24da8deeb50118271c9435972791ef05ec003b1/opentelemetry/proto/logs/v1/logs.proto#L192
profile_id
is required: https://github.com/open-telemetry/opentelemetry-proto/blob/f24da8deeb50118271c9435972791ef05ec003b1/opentelemetry/proto/profiles/v1development/profiles.proto#L247
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean making the profiles proto profile_id
field optional neither making it accepting zeroed values.
My point is, should we allow using the ProfileID(...)
function with zeroed byte array? Taking the examples you showed, trace_id
and span_id
are optional fields in the Logs proto, so they can be empty at some point, but on the other hand they're also required on the traces proto.
That said, IMO:
- If having empty profile IDs (as fields of other protos) is/will be a possibility, so the function shouldn't restrict creating empty profiles IDs. It allows users filtering by, and cleaning the field values if they want:
span_id == SpanID(0x0000000000000000)
,set(span_id, SpanID(0x0000000000000000))
. profile_id
path setters should ensure that non-zeroed profiles are set to the profiles proto, not the converter function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I get you, makes sense! :)
Removed the check for the converter functions and create a new PR for not allowing to set empty ProfileIDs via the OTTL accessor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New PR to disallow setting empty profile ids: #39659
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add an e2e test for the new converter? similar to the existing SpanID.
Description
Add the OTTL function
ProfileID()
, which is currently missing.