update semantic conventions to v1.19 #965
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
============================================
- Coverage 82.95% 82.33% -0.63%
- Complexity 2165 2179 +14
============================================
Files 281 283 +2
Lines 6195 6220 +25
============================================
- Hits 5139 5121 -18
- Misses 1056 1099 +43
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
taking a lead from the go SIG, who seem to be using semconv the most, I've split semantic conventions up into smaller files representing top-level semconvs. I can't find any usage of the various AttributeValues being used, and other SIGs no longer generate them, so removing them
|
asked a question on spec channel about how to organize semconv - it looks like other SIGs are starting to create "resource" + "everything else" which is different to what we're doing. |
src/API/Trace/functions.php
Outdated
| @@ -35,7 +35,7 @@ function trace(SpanInterface $span, Closure $closure, iterable $args = []) | |||
| return $c(...$a, ...($a = [])); | |||
| } catch (Throwable $e) { | |||
| $s->setStatus(StatusCode::STATUS_ERROR, $e->getMessage()); | |||
| $s->recordException($e, [TraceAttributes::EXCEPTION_ESCAPED => true]); | |||
| $s->recordException($e, [EventAttributes::EXCEPTION_ESCAPED => true]); | |||
There was a problem hiding this comment.
| $s->recordException($e, [EventAttributes::EXCEPTION_ESCAPED => true]); | |
| $s->recordException($e, ['exception.escaped' => true]); |
Can we drop the open-telemetry/sem-conv from the API-package to avoid breaking the API on sem-conv updates?
There was a problem hiding this comment.
That's a good question. If we did drop it, what is the point of having the sem-conv package? I've noticed that other SIGs version their semantic convention package the same as the spec version (eg 1.19.0 in this case), and presumably then pin APIs etc to that version. I have a feeling that might be very painful if packages are dependant on different versions, though.
The other thing some SIGs are doing is moving all attributes into two classes: ResourceAttributes, and SemanticAttributes, the latter being "everything except resources". That should at least solve the problem of things moving about as the spec evolves.
There was a problem hiding this comment.
IMO contrib/instrumentation packages are the main consumers of the sem-conv package.
(Java added removed attributes to the template to avoid a BC-break open-telemetry/opentelemetry-java@188210b, I like this approach because it makes pinning to a specific version in instrumentation packages obsolete when specifying the schema url as literal instead of relying on TraceAttributes::SCHEMA_URL.)
Regarding the API dependency: even if we resolve the BC-break I don't see a reason to keep the sem-conv dependency in the API package, we only use it for a small helper function. Iff ::recordException() / exception.escaped are changed in v1 in a breaking way, we should deprecate the trace() function. (The SDK uses literals for the other 'exception.' attributes in ::recordException() too / any breaking change would very likely also affect the SDK.)
There was a problem hiding this comment.
Thanks for pointing that out - adding removed items to the template and marking them as @deprecated is a good idea. I've loosely copied java's deprecation pattern, and decided to remove the new semver classes I've added, since java hasn't done anything yet and I'd be more comfortable if they go first and we copy.
java has not progressed with these yet, so I think we should wait until we see what they do :) allow deprecations of moved/removed attributes so that we don't break things across different semconv versions (similar to how java does it, except I moved them into a file which is imported
* Fix typo "interal" -> "internal" * Remove semconv from API deptrac ruleset Removed in #965.
* Fix typo "interal" -> "internal" * Remove semconv from API deptrac ruleset Removed in open-telemetry/opentelemetry-php#965.
* Fix typo "interal" -> "internal" * Remove semconv from API deptrac ruleset Removed in open-telemetry/opentelemetry-php#965.
|
Hi @brettmc, I noticed have many const be deprecated, like the |
|
The ones that we deprecate (from https://github.com/open-telemetry/opentelemetry-php/blob/main/script/semantic-conventions/templates/trace_deprecations.php.partial) are attributes that were removed from upstream, and we add them back with the deprecation notice to try to avoid breaking changes for users. Because they were totally removed from semconv (rather than being deprecated there), we don't have further info about which attributes to use instead. In the case of |
|
@brettmc I got it, Thank you for your detailed reply. The |
taking a lead from the go SIG, who seem to be using semconv the most, I've split
semantic conventions up into smaller files representing top-level semconvs.
I can't find any usage of the various AttributeValues being used, and other SIGs
no longer generate them, so removing them
[BREAKING] some attributes have moved, notably EXCEPTION_ESCAPED from TraceAttributes to EventAttributes, which requires a corresponding fix in some contrib packages.