-
Notifications
You must be signed in to change notification settings - Fork 516
[SDK] Implement EnvEntityDetector for OTEL_ENTITIES parsing (#3652) #3795
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
[SDK] Implement EnvEntityDetector for OTEL_ENTITIES parsing (#3652) #3795
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3795 +/- ##
==========================================
- Coverage 89.93% 89.91% -0.01%
==========================================
Files 225 225
Lines 7163 7163
==========================================
- Hits 6441 6440 -1
- Misses 722 723 +1 🚀 New features to boost your workflow:
|
1d75005 to
f7a5844
Compare
4e42cbc to
296d576
Compare
…emetry#3652) Changes: Add EnvEntityDetector class to parse OTEL_ENTITIES environment variable Parse entity format: type{id_attrs}[desc_attrs]@schema_url Support percent-encoding for reserved characters Handle duplicate entities, conflicting attributes, and malformed input per spec Include comprehensive test coverage Fix schema URL handling to pass to Resource Details: EnvEntityDetector::Detect() reads OTEL_ENTITIES and returns Resource with parsed attributes ParseEntities() splits input by semicolons and parses each entity definition ParseSingleEntity() extracts type, id_attrs, desc_attrs, and schema_url from entity string ParseKeyValueList() parses comma-separated key=value pairs PercentDecode() decodes percent-encoded values BuildEntityIdentityKey() creates stable key for duplicate detection Error handling: malformed entities skipped, duplicates use last occurrence, conflicts log warnings Implements entity propagation spec: https://opentelemetry.io/docs/specs/otel/entities/entity-propagation
296d576 to
aeec00f
Compare
marcalff
left a comment
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.
LGTM, thanks for the contribution
dbarker
left a comment
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.
Thanks for the PR! One minor request - please move this resource detector and tests to the resource_detectors folder.
https://github.com/open-telemetry/opentelemetry-cpp/tree/main/resource_detectors
| std::string{opentelemetry::common::StringUtil::Trim(entity_str.substr(cursor + 1))}; | ||
|
|
||
| // TODO: Use a proper Schema URL validator when available. | ||
| if (out.schema_url.empty() || out.schema_url.find("://") == std::string::npos) |
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 see the TODO mentions that schema validation needs work, but right now it's too loose - it accepts anything with :// in it. We should at least check that the scheme is http or https like the spec requires.
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.
From:
https://opentelemetry.io/docs/specs/otel/entities/entity-propagation/#validation-requirements
Schema URL, if present, MUST be a valid URI
so this should not be restricted to http or https only.
But I agree, this probably needs revision, as one of the examples given:
# Multiple entities with schema URL
OTEL_ENTITIES="service{service.name=my-app,service.instance.id=instance-1}[service.version=1.0.0]@/schemas/1.21.0;host{host.id=host-123}[host.name=web-server-01]"
does not even use a scheme with ://.
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 did it for now to validate general URIs. Stricter schema URI checks can be updated when new guidance is available. Thanks
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.
LGTM. Have couple of non-blocking comments - can be handled in current PR or as separate improvement.
…n EnvEntityDetector - Use nostd::get_if instead of nostd::get for safe string access - Use direct variant comparison instead of string conversion - Handles type mismatches correctly (variants with different types compare as unequal)
marcalff
left a comment
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.
LGTM, thanks for the contribution
Related #3652
Changes:
Details:
EnvEntityDetector::Detect()reads OTEL_ENTITIES and returns Resource with parsed attributesParseEntities()splits input by semicolons and parses each entity definitionParseSingleEntity()extracts type, id_attrs, desc_attrs, and schema_url from entity stringParseKeyValueList()parses comma-separated key=value pairsPercentDecode()decodes percent-encoded valuesBuildEntityIdentityKey()creates stable key for duplicate detectionError handlingmalformed entities skipped, duplicates use last occurrence, conflicts log warningsImplements entity propagation spec:
https://opentelemetry.io/docs/specs/otel/entities/entity-propagation/