Skip to content
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

Rename old env variables that use TRACE_ with TRACES_ #1382

Merged
merged 5 commits into from
Feb 2, 2021

Conversation

bogdandrutu
Copy link
Member

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

Follow-up after #1362

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but OTEL_TRACES_SAMPLER sounds very unnatural. Are we sure we don't want to go the other way round and use TRACE_ everywhere? EDIT: Same with METRICS_EXPORTER vs METRIC_EXPORTER: Singular sounds more natural (and is one letter shorter 😃)

@bogdandrutu
Copy link
Member Author

@Oberon00 :))

Base automatically changed from master to main January 27, 2021 21:16
@carlosalberto carlosalberto added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Jan 27, 2021
@carlosalberto
Copy link
Contributor

Thanks for the catch! Was wondering whether we had forgotten something or not :)

Are we sure we don't want to go the other way round and use TRACE_ everywhere?

Not a strong opinion from me, as long as we stick to one or the another. I doubt there will be a perfect solution :/

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR is already by definition a bike-shedding discussion, I don't feel about making this a "Request changes": I strongly object to the plural form, which sounds like bad English to me. If we want consistency we should go with singular (both TRACE_ and METRIC_) everywhere.

@carlosalberto
Copy link
Contributor

I strongly object to the plural form

@tigrannajaryan Any opinion on this? You were a big supporter of the plural as (IIRC) that's what is used in the Collector.

(I'm fine either way, btw)

@carlosalberto carlosalberto added area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Jan 28, 2021
@tigrannajaryan
Copy link
Member

The reason I suggest to use plural is we have a lot of code in Collector that uses plural which is also a public API of the Collector. If we were to change it then it will be a breaking change for third parties that rely on it (we are Beta - so formally we are allowed to do it). Plural form is also used in Collector config files, so it will be a breaking change for end users as well.

Changing back to singular is at least inconvenience for third-party people who we will break. IMO, it is not worth it.

Singular sounds more natural (and is one letter shorter 😃)
I strongly object to the plural form, which sounds like bad English to me

@Oberon00 to be fair, none of what you listed seems like a good enough justification to me. I may be wrong.

@Oberon00
Copy link
Member

OK, fair enough. But then isn't leaving everything as it is a good option as well?

@Oberon00 Oberon00 dismissed their stale review January 28, 2021 16:42

For practical reasons, it is already too much effort to rename the older enviornment variables.

CHANGELOG.md Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

But then isn't leaving everything as it is a good option as well?

Well, we already had a few mismatches here and there (METRIC vs METRICS and SPAN vs TRACE) :( So sadly we had to change things one way or another. Oh well...

Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (after conflict resolved in CHANGELOG.md)

@tigrannajaryan
Copy link
Member

OK, fair enough. But then isn't leaving everything as it is a good option as well?

I think we should aim for consistency and use plural "everywhere". Unfortunately "everywhere" is not really 100% everywhere; a couple places in OTLP proto declarations we cannot touch because it will break the proto. But vast majority of Otel spec and code can use plural. Today it is a mix of plural and singular depending on the repo or are of work.

@jkwatson
Copy link
Contributor

I don't like the plurals, and this will break every language SDK out there, correct? So, we fix the collector, or we force all language maintainers to change? Seems like changing in one place (the collector) would be a much smaller impact.

@tigrannajaryan
Copy link
Member

I don't like the plurals, and this will break every language SDK out there, correct? So, we fix the collector, or we force all language maintainers to change? Seems like changing in one place (the collector) would be a much smaller impact.

I would risk and guess that very little real users currently depend on SDK's env variable today as opposed to many users actually using Collector in production and a large number of developers . So the impact is not really smaller, it is bigger if we touch the Collector. Again, I may be wrong, I am probably not aware of production uses of SDKs, happy to be corrected.

I do not mind doing the work and renaming everything in the Collector (it is easy to do) but the implications of the change I think are too big to do it.

If we are not able to agree on a breaking change then this needs to be handled gracefully by supporting both plural and singular for a while (highly undesirable and a lot of extra work).

@Oberon00
Copy link
Member

How about supporting both plural and singular in the collector? Shouldn't be hard to implement either.

@carlosalberto
Copy link
Contributor

I don't like the plurals, and this will break every language SDK out there, correct?

Well, sadly we already broke them by renaming a few other things from _SPAN_ to _TRACES_, so this already happened 😢

How about supporting both plural and singular in the collector? Shouldn't be hard to implement either.

That's just avoiding the problem IMHO 😅

I personally suggest sticking to _TRACES_ as originally discussed recently, even if it's an imperfect solution.

@Oberon00
Copy link
Member

Oberon00 commented Jan 28, 2021

That's (in a way 😉 ) solving the problem. Only the singular would be spec'd and documented, and existing collector user's config continues to work. Language SDKs are free to do the same thing, or just support the final name.

@tigrannajaryan
Copy link
Member

I don't like the plurals, and this will break every language SDK out there, correct?

Well, sadly we already broke them by renaming a few other things from _SPAN_ to _TRACES_, so this already happened 😢

This is a good point. Since we already did this and this needs to be done regardless of plural/singular debate I think there is no point worrying about breaking SDKs anymore. SDKs are already broken. I do not see why we should also break the Collector.

@tigrannajaryan
Copy link
Member

That's (in a way 😉 ) solving the problem. Only the singular would be spec'd and documented, and existing collector user's config continues to work. Language SDKs are free to do the same thing, or just support the final name.

I don't understand what is solving the problem. Please clarify.

@Oberon00
Copy link
Member

I meant supporting both the previously existing plural names and the (hypothetical) new singular names. #1382 (comment)

@trask
Copy link
Member

trask commented Jan 28, 2021

I don't like the plurals

fwiw, the plural env var names (OTEL_TRACES_EXPORTER, OTEL_TRACES_SAMPLER) feel awkward to me also

Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a foreign language speaker both plural and singular are the same to me :) Thus consistency wins.

bogdandrutu and others added 2 commits January 29, 2021 10:08
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@jsuereth
Copy link
Contributor

jsuereth commented Jan 29, 2021

Is the churn caused to collector users really necessary here? If we discover issues like this post 1.0 what's the plan then?

I understand a desire for consistency, but in this case I think it is affecting real users. I'd like to echo @tigrannajaryan here. We need to be far more careful of changes NOW if we really want our 1.0 to be stable. This is exactly the kind of churn that can cause delays for minimal gain. I'd like to understand the plan for how to handle this kind of inconsistency post 1.0.

Why not allow both variables with a migration period so there's no IMMEDIATE churn.

@carlosalberto
Copy link
Contributor

Why not allow both variables with a migration period so there's no IMMEDIATE churn.

We already broke them for the SDKs by renaming a few inconsistencies already, i.e. OTEL_EXPORTER becoming OTEL_TRACE_EXPORTER and OTEL_METRICS_EXPORTER. So changing them IMHO it's ok but we need to make sure that whatever we choose stays stable (just to be on the safe side).

@tigrannajaryan Are there any environment variable that uses TRACES in the collector? IIRC we only use traces for a config like

  pipelines:
    traces:
      receivers: [otlp, opencensus, jaeger, zipkin]

So I'm personally fine with the Collector using traces (as a single word) and all the environment variables in the API/SDK using _TRACE_ instead.

@tigrannajaryan
Copy link
Member

@tigrannajaryan Are there any environment variable that uses TRACES in the collector?

No, there is none in the Collector.

@carlosalberto
Copy link
Contributor

carlosalberto commented Feb 1, 2021

Based on today's conversation at the Maintainers meeting call, I'd strongly suggest to go with TRACES even if it feels ugly in some cases/combinations, in order to go dead simple and have the same plural traces everywhere (and, hopefully, we finally reach stability for env variables).

In that regard, we do have enough approvals already, btw ;)

cc @jsuereth

@jsuereth
Copy link
Contributor

jsuereth commented Feb 2, 2021

@carlosalberto I think my point on stability was well discussed. Given the consideration, and the desire to still merge this, that's ok. Hopefully we'll change future behavior in this regard, and I think the right discussions have kicked off. Don't hold off on this for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet