Remove MILLIS from BatchSpanProcessor variables and define semantics for...

[anuraaga]

… duration values.

Fixes #1312

Changes

Removes MILLIS from BatchSpanProcessor time variables in favor of defining durations to always be interpreted as milliseconds.

[anuraaga]

@mwear I'm imagining Endpoint to also be added to this list after #1234 (awesome PR number!)

[yurishkuro]

I am not sure this is worth it. In Go this can be parsed with stdlib, but that's not always the case in other langs, so it introduces an extra burden on SDK implementations with not a lot of benefits.

[anuraaga]

@yurishkuro We're discussing on #1312. @jsuereth has started working on adding it to all the languages and not sure how best to proceed with it without a definition of the unit strings. open-telemetry/opentelemetry-java#2439 I'm personally fine with anything that makes the variable names consistent :)

[jsuereth]

FYI - I didn't notice you already had this PR. I described this a bit differently: https://github.com/open-telemetry/opentelemetry-specification/compare/master...jsuereth:wip-duration-defaults?expand=1

I think your specification is a lot smaller, but is missing the bit about PositiveInteger, i.e. I think we also need to limit the numeric value so it's reasonable to contribute a parser in all languages.

Suggested change:

Any value that represents a duration, for example a timeout, MUST interpret a purely positive integer value as a number of milliseconds. Formatted strings with a positive integer followed immediately by a unit string MAY be interpreted for the following units:

[carlosalberto]

Formatted strings with a positive integer followed immediately by a unit string MAY be interpreted for the following units:

I'm afraid that languages not supporting this specific sentence will soon be approached by users asking: "Hey, why doesn't this work? In Go it does but it doesn't in X language", converting it the de facto required.

(I know this is a small piece of work - but if we allow this to creep, we then may allow other 20 small additions and drag the release more than it is - and, well, we can easily add support for this parsing by the time we do the next release, which should include stable metrics :) )

[jsuereth]

I understand the concern.

1. I'm not sure how else you would phrase this in a way to ALLOW Go's handling of duration and also allow parsing units in the future to become required (post-GA). If you have a recommendation for how we allow this change to occur post-GA, let us know. Currently, I think having _MILLIS in env names actually prevents this format from being a good idea post-GA so at a minimum I'd like to remove that from the ENV variable.
2. I think it's on the maintainers of those langauge maintainers to PUNT the request or ask community for PRs. i.e. This spec change is intended to NOT force languages to implement parsers, but allow them to come post GA, but will need discipline from maintainers to focus on more important issues.
3. I'm more than happy to contribute parsers for this to languages which need it, feel free to assign it to me. Anything I can do to help maintainers focus on stable metrics :)

[bogdandrutu]

@jsuereth looks like you requested changes, is there any concrete issue/reason that we need to solve?

[jsuereth]

@bogdandrutu Yeah, here's the quote:

FYI - I didn't notice you already had this PR. I described this a bit differently: https://github.com/open-telemetry/opentelemetry-specification/compare/master...jsuereth:wip-duration-defaults?expand=1

I think your specification is a lot smaller, but is missing the bit about PositiveInteger, i.e. I think we also need to limit the numeric value so it's reasonable to contribute a parser in all languages.

Suggested change:

Any value that represents a duration, for example a timeout, MUST interpret a purely positive integer value as a number of milliseconds. Formatted strings with a positive integer followed immediately by a unit string MAY be interpreted for the following units:

[carlosalberto]

I think your specification is a lot smaller, but is missing the bit about PositiveInteger, i.e. I think we also need to limit the numeric value so it's reasonable to contribute a parser in all languages.

Based on the comments from @jkwatson and @yurishkuro (and how, even if such parsers are contributed anytime soon, this could add required cycles to the SIGs), I still think the best option is to:

1. Remove the _MILLIS prefix and define the expected/recommended value range, all in MS by default (as part of this issue/related PR).
2. Depending on the parser effort in the main SIGs, adopt the actual parsing of values (in a different issue marked as Allowed-for-GA, which means that it could still happen before Tracing goes stable - else, have it after the release).

[anuraaga]

@jsuereth Sorry for missing the comment about positive numbers! Will address

@carlosalberto So do you want to leave out discussion of duration units in this PR? What happens if different languages end up with different units? Maybe a low enough probability not to matter though.

[carlosalberto]

So do you want to leave out discussion of duration units in this PR?

Not quite, but have milliseconds being the default. IIRC the parsers @jsuereth mentioned take milliseconds as the default, unless I'm wrong?

[jsuereth]

@carlosalberto The parser I contributed to Java (and plan for other languages) takes milliseconds as the default.

I think your plan for "allow-for-GA" with specific changes is exactly in line with what I was hoping to accomplish!

Parsers can be added when it makes sense to do so.

[anuraaga]

@carlosalberto Ah ok - I just added the note about negative numbers not being supported. I think my wording of MUST vs MAY captured the aspect of the default being numeric milis, let me know if this is still missing anything.

[carlosalberto]

Based on the discussion we had today at the Spec call, we decided to go with:

1. Remove _MILLIS for all related environment variables and have milliseconds as the actual default unit.
2. Fill a follow-up issue (allowed for GA) to have the duration unit parsing in place. We will merge this as soon as the main SIGs have support in place. See Add duration unit parsing for environment variables #1351

@anuraaga Please amend appropriately ;)

[anuraaga]

@carlosalberto Ok updated to only talk about the number.

[carlosalberto]

@anuraaga Thanks for the update!

@open-telemetry/specs-approvers please review/approve ;)

[tedsuo]

LGTM. Appreciate the simplification and, dare I say it, standardization.