-
Notifications
You must be signed in to change notification settings - Fork 893
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
Update default for span limits to 128, 1000 seems off #1419
Update default for span limits to 128, 1000 seems off #1419
Conversation
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
603f0f5
to
5345631
Compare
I can tell you where the 1000 comes from: #182 (comment) It was chosen because
128 was also discussed as limit but thrown out because it might prevent valid use cases, see #182 (comment) As this is only an experimental part of the spec, I think it's OK to rush a change here, but if combined with #1416, I would oppose it. |
I still think 1000 is a good default to allow for 99.9% of all use cases if this is not configurable. With #1416 making configurability a MUST, however, going for a default of 128 should be fine since users have the option to adapt it if they're doing something special that would require them to go beyond that. |
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. I am a bit confused then too why this became 1000, given that the 1000 was based on the assumption that it is not configurable while the 1000 appears exactly as default value for a configuration option.
IIRC the original PR made span limits optionally configurable, and stated that the default or unconfigurable value is 1000. FWIW 1000 was chosen over 128 because 128 could be ambiguous as some code-level limitation (e.g. right above an 8-bit signed integer), and 1000 is clearly a human-defined limit. I'm not really sure why 128 vs 1000 (or visa versa) necessitates a change. but not here to debate the change, just add clarity. |
…#1419) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
128 is based on the value used in OpenCensus and it was not a problem there. See https://github.com/census-instrumentation/opencensus-java/blob/3d7f0b511dea392bbf68ee5268dd95857552dd87/api/src/main/java/io/opencensus/trace/config/TraceParams.java#L36
The limits for attributes and links were smaller, but I applied a max(of_all) function.