Opentelemetry-specification: OTLP Exporter timeout env var not consistent with batch span processor

[anuraaga]

BatchSpanProcessor suffixes with MILLIS

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-environment-variables.md#batch-span-processor

OTLP doesn't

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/protocol/exporter.md#configuration-options

I guess they should both be suffixed with MILLIS

[carlosalberto]

I'm fine either way as long as we stick to one single pattern.

[carlosalberto]

Marking this as required-for-GA as we either change it now or never.

[bogdandrutu]

It would be good if we remove the "MILLIS" from the variable names and we accept "duration" as the value which includes the suffix of the time unit. This is a good experience for our users.

E.g.:
"5000" - for 5 seconds in the millis format
vs
"5s" - for 5 seconds in the duration format

[jsuereth]

FYi: Apache licensed parser for duration (in java): https://github.com/lightbend/config/blob/7c44daf5f5a42e51f702f3ababbd27f6177d3858/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java#L735

And a specification of what's supported: https://github.com/lightbend/config/blob/master/HOCON.md#duration-format

I'm happy to help contribute an adapted version of this across APIs/SDKs that don't already support duration parsing, if it's reasonable to do so.

[jkwatson]

@jsuereth We'd really like to keep our SDK dependencies as minimal as possible, so I would strongly resist pulling in a parser library without shading/repackaging it, just FYI.

[jsuereth]

@jkwatson My suggestion was to re-implement just that portion of code (~60 lines).

[anuraaga]

I'm mostly worried about the feasibility of implementing somewhat complicated parsing across many languages, though I do like the format in general (lightbend config is awesome!). If it seems practical that would work great.

[jsuereth]

So the complicated part is reporting errors, the actual parser amounts to this:

    private static String getUnits(String s) {
        int i = s.length() - 1;
        while (i >= 0) {
            char c = s.charAt(i);
            // Note: Here we can just look for any character not in our approved token list for time units we support.
            if (!Character.isLetter(c))
                break;
            i -= 1;
        }
        return s.substring(i + 1);
    }    

And then chunking the original string between the units-portion + the non-units portion. Most of the complication from typesafe-config is dealing with Java's String bugs, like trim not working w/ well with full unicode support. This says nothing of the fact that environment variables inherently have unknown encoding, nor did I see any mention in the spec of whether odd formats, like Windows-1252 or CP437

As I said in the meeting, i think if we spec that this string has to contain specific characters (e.g. EXACTLY space, 'm', 's' and/or normal digit characters 0-9) then we can write a simple parser relatively quickly that can give good error messages. I'm more than happy to send out a few PRs for languages that don't already support this to show what I mean. Besides Java, are there known languages I should target?

[jsuereth]

So I was able to add a utility for java in ~60 LOC (although it may grow as we improve error messages). See: https://github.com/open-telemetry/opentelemetry-java/pull/2439

I'm more than happy to write up a specification on Duration format and walk across the languages making sure we have utility methods or built-in language support. Is that a reasonable next step here?

[anuraaga]

I'm +1 for that @jsuereth thanks a lot! I think the first step is to create a PR opposite of #1319 that removes the MILLIS suffix and adds a general point somewhere that all duration vaules, such as timeouts, accept millisecond values. A followup could then expand that to support duration strings.

[carlosalberto]

What about declaring that all duration env-vars params use (default to) milliseconds and after GA we (easily) add support for specifying units?

[anuraaga]

@carlosalberto Yeah I think that's what I said :P

[yurishkuro]

I am in favor of keeping MILLIS. Parsing durations requires extra code to maintain, I see very little benefits to it. Seconds are trivial to represent as millis, and all other units are probably largely irrelevant for the parameters that actually need durations.