Adding environment variables for event and link attribute limits

[codeboten]

Fixes #1750

Changes

Adding environment variable definitions for span event and link attributes.

[carlosalberto]

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

[tigrannajaryan]

Somewhat meta, but I am a bit worried about the proliferation of the env variables. I wonder if this is desirable or we should only have basic capabilities controlled by env variables and the rest requires writing actual code? Taken to the extreme we can start coding the entire SDK behavior in the env variables. Is there a good way to think about what should be an env variable and what shouldn't?

[codeboten]

Is there a good way to think about what should be an env variable and what shouldn't?

I agree, it would be great to have guidelines on this. Specifically around this PR, I think there should be equivalent variables for all available limits configuration, otherwise i could imagine this being confusing as a user. Why would some limits be configurable via env variables vs others not.

I suspect these will become less important once there's a configuration format available for otel components.

[iNikem]

We cannot forbid language SIGs to provide configuration via environment variables. And as soon as one SDK has them, it is a good idea to standardise them across languages. But then, as @codeboten mentioned, it will be confusing to do that only for a subset of configuration options.

[carlosalberto]

I agree with @tigrannajaryan's point (hence my initial idea of having a single env var to handle all the attribute limits), but for this case I think it does make sense to add them right now.

Hopefully the new Instrumentation group will be able to work out further details like this (as they are part of the configuration area). cc @tedsuo

[carlosalberto]

Also, unless we have some opposition, let's merge this by early Monday, so allow more time for people to review/approve this.

[jtmalinowski]

Prefix with OTEL_SPAN_ for consistency? Otherwise the name makes it seem like it applies to all events.

[carlosalberto]

#1751 (comment)

[jtmalinowski]

Surprising, but okay if it's decided already.

[tigrannajaryan]

#1130 seems to be a very similar PR for different things to limit.

My initial worry stands: there seems to be a proliferation of env variables. Is there a point where this becomes unmanageable? Are we aiming at having full behavior configuration via env variables? Taken to the extreme we need a Turing-complete language and supply programs in env variables that modify the default behavior of SDK. I am not convinced this is the right direction.

Perhaps we need to support a config file and have one env variable that points to the config file to use?

[tigrannajaryan]

#1130 seems to be a very similar PR for different things to limit.

My initial worry stands: there seems to be a proliferation of env variables. Is there a point where this becomes unmanageable? Are we aiming at having full behavior configuration via env variables? Taken to the extreme we need a Turing-complete language and supply programs in env variables that modify the default behavior of SDK. I am not convinced this is the right direction.

Perhaps we need to support a config file and have one env variable that points to the config file to use?

@open-telemetry/technical-committee and @open-telemetry/specs-approvers: I would like your opinion before this PR gets merged.

I am not against this particular PR, but would like to understand if we need some best practices around no-code configuration and whether what we do with env variables is acceptable long term.

[carlosalberto]

@tigrannajaryan we decided during the last SIG call to go ahead with these two aforementioned PRs, while creating #1773 to track the configuration dilemma. Let me know if that works for you ;)

[tigrannajaryan]

@tigrannajaryan we decided during the last SIG call to go ahead with these two aforementioned PRs, while creating #1773 to track the configuration dilemma. Let me know if that works for you ;)

SGTM.