-
Notifications
You must be signed in to change notification settings - Fork 11
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
Don't enforce span name length limit for spans the SDK logs #1125
Conversation
74a2e3d
to
cc3007f
Compare
a3f657c
to
492b0c2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
+ Coverage 82.84% 82.93% +0.09%
==========================================
Files 484 484
Lines 11041 11041
Branches 1699 1699
==========================================
+ Hits 9147 9157 +10
+ Misses 1152 1144 -8
+ Partials 742 740 -2
|
cc3007f
to
de57070
Compare
492b0c2
to
dce8d22
Compare
dce8d22
to
486b738
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this PR will be closed in 10 days. |
d788acc
to
5f29880
Compare
8ec1a46
to
c336053
Compare
c336053
to
61118ec
Compare
61118ec
to
75827dc
Compare
3ba227a
to
b48775a
Compare
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.
IMO the SDK should still enforce a limit that prevents span names from being unbounded, but I'd be happy for it to be longer than what we allow for customers. Is increasing the span name length for customers so that only one limit applies universally also a simpler option?
(This all assumes the backend hasn't made any assumptions about span name length, too)
I wanna say there's already an implicit length that is enforced at the OTel SDK level (there is for most span things), so if we can configure that to what we want, lets use that. If not, I'll add another mechanism for this. What's reasonable for us - 2K chars? |
Sounds reasonable! |
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
Merge activity
|
38bbc97
to
c25d6fb
Compare
c25d6fb
to
27d973f
Compare
Goal
Names of spans we record internally can be longer than than the custom performance traces that we have a name length restriction for. Make the validate not apply to spans logged by the SDK, i.e. internal = true.