Allow span limits to be unset #1830
Conversation
owais
requested review from
a team,
aabmass and
lzchen
and removed request for
a team
owais
force-pushed
the
allow-to-unset-limits
branch
from
1fd299e to
2f0ef46
Compare
|
Interesting, was there any use-case that inspired this change? |
|
Yes, these limits do not work for some Splunk customers at least so Splunk distributions tend not to set them by default and let the users decide. |
owais
force-pushed
the
allow-to-unset-limits
branch
from
2f0ef46 to
ea0f348
Compare
Ths spec recommends most span limits to default to 128 and allow users to change it via environment variables. It does not dictate that limits should always be set though. So users should be able to remove these limits. This commit accepts `"none"` as a valid value for the limit config environment variables.
owais
force-pushed
the
allow-to-unset-limits
branch
from
ea0f348 to
5e6a893
Compare
| _DEFAULT_SPAN_LIMIT = "128" | ||
|
|
||
|
|
||
| def _get_limit_from_env(limit_name) -> Optional[int]: |
There was a problem hiding this comment.
Instead of having an "unset" value, should we maybe allow users to specify a limit manually themselves through the span api? This follows the whole hardcoded -> env var -> default value priority architecture that we use in a lot of places for configuration.
There was a problem hiding this comment.
That would be very nice to have. Any configuration that happen automatically on importing modules should be moved to SDK initialization IMO. That said, this PR is merely allowing setting the limits to "none" in addition to integer values.
Created an issue to implement what you suggested: #1838
Will work on it soon but we shouldn't rush it into the release tomorrow.
|
superseded by #1839 |
Description
Ths spec recommends most span limits to default to 128 and allow users
to change it via environment variables. It does not dictate that limits
should always be set though. So users should be able to remove these
limits. This commit accepts
"none"as a valid value for the limitconfig environment variables.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: