Add support for span collection limit via env vars #1377

LetzNico · 2020-11-12T00:57:11Z

This adds support for the following env variables:

OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT (Maximum allowed span attribute count)
OTEL_SPAN_EVENT_COUNT_LIMIT (Maximum allowed span event count)
OTEL_SPAN_LINK_COUNT_LIMIT (Maximum allowed span link count)

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #1364

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update (not sure?)

How Has This Been Tested?

Unit tests added

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated (Needed? If yes, where?)

LetzNico · 2020-11-15T23:46:15Z

Exporter builds do not pass because a test asserting to True logs an errors... I'm not sure I get why it happens while related code has not been updated since mid-october 🤷‍♂️

lzchen · 2020-11-16T14:33:22Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -429,8 +427,10 @@ def __init__(
        instrumentation_info: InstrumentationInfo = None,
        record_exception: bool = True,
        set_status_on_exception: bool = True,
+        attribute_count_limit: int = None,


I believe tracer.start_as_current_span() and tracer.start_span() needs to have these parameters as well, since that is the suggested way to construct spans through our api.

that is an interesting question. If we do that it will violate the API.

There isn't actually a requirement to expose this configuration via the span methods: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#limits-on-span-collections.

This configuration could be handled in the constructor of the TracerProvider, which forwards those values to the Span.

Or we can limit configuration to be only done through environment variables, similarly to propagators.

that's fine, although something I generally am not a huge fan of. My personal taste for programmatic APIs is not a blocker :)

FWIW my preference comes from the desire for linter / syntax checkers to verify that the method I'm calling exists. It's hard to know whether an environment variable is still supported or not programmatically: you just hope there's no backwards incompatible changes.

I agree, as a user I would find it difficult to find out what env vars are available. Perhaps more can be discussed here. For now, I am fine with leaving the programmatic part out of this PR.

toumorokoshi

Thanks for the changes! Overall the idea looks good. I think we do need some way to allow the user to manually wire in the values (sdk.TraceProvider would be good), and if possible we only have to query configuration once in the lifetime of the application to change that setting.

toumorokoshi · 2020-11-17T14:48:48Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -429,8 +427,10 @@ def __init__(
        instrumentation_info: InstrumentationInfo = None,
        record_exception: bool = True,
        set_status_on_exception: bool = True,
+        attribute_count_limit: int = None,


that is an interesting question. If we do that it will violate the API.

There isn't actually a requirement to expose this configuration via the span methods: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#limits-on-span-collections.

This configuration could be handled in the constructor of the TracerProvider, which forwards those values to the Span.

LetzNico · 2020-11-21T23:38:55Z

Hi! Settled down with:

Configuration retrieval in the TracerProvider initialization
Environment variables as only way to set those limits

Up for review, do not hesitate if this implementaion is not satisfying (yet!)

lzchen · 2020-11-24T03:35:45Z

I think from this comment, we only want these configurations to be modified through environment variables, not through the parameters of the constructor.

LetzNico · 2020-11-24T09:18:59Z

I think from this comment, we only want these configurations to be modified through environment variables, not through the parameters of the constructor.

But aren't tracers and spans supposed to only be created through dedicated methods like (get_tracer(), start_as_current_span() and start_span())? I believe that those constructors parameters are only manipulated by the tracer, derived from the tracer provider, which gets those config from env.

If that is not satisfying, do you think an okay approach would be to simply change those three lines to retrieve from env? https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L52

lzchen · 2020-11-24T14:25:44Z

But aren't tracers and spans supposed to only be created through dedicated methods like (get_tracer(), start_as_current_span() and start_span())?

lzchen · 2020-11-24T14:27:26Z

But aren't tracers and spans supposed to only be created through dedicated methods like (get_tracer(), start_as_current_span() and start_span())?

You are correct, however adding these as parameters to those functions will break the api definitions as [mentioned above](#1377 (comment). I think the consensus is to not expose these at all to be manually configured through the constructors, but only configured through environment variables similar to [propagators](https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/propagators/__init__.py#L129.

toumorokoshi

LGTM! Thanks.

toumorokoshi · 2020-11-24T16:45:33Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -907,6 +915,16 @@ def __init__(
        if shutdown_on_exit:
            self._atexit_handler = atexit.register(self.shutdown)

+        self.span_attribute_count_limit = Configuration().get(


at this point, you've almost done all the work to make this programmatically configurable: just make each of these defaults customizable, or some combination there of.

But not an issue if you choose not to do so.

LetzNico · 2020-11-29T17:31:17Z

Hi @lzchen ! Sorry, I have not taken the time to get back to this.

But aren't tracers and spans supposed to only be created through dedicated methods like (get_tracer(), start_as_current_span() and start_span())?

You are correct, however adding these as parameters to those functions will break the api definitions as [mentioned above](#1377 (comment). I think the consensus is to not expose these at all to be manually configured through the constructors, but only configured through environment variables similar to [propagators](https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/propagators/__init__.py#L129.

Understood! I have tried to write something like this, which basically is like replacing doing that if I'm not mistaken:

-MAX_NUM_ATTRIBUTES = 1000
-MAX_NUM_EVENTS = 1000
-MAX_NUM_LINKS = 1000
+SPAN_ATTRIBUTE_COUNT_LIMIT = Configuration().get(
+    "SPAN_ATTRIBUTE_COUNT_LIMIT", 1000
+)
+SPAN_EVENT_COUNT_LIMIT = Configuration().get(
+    "SPAN_EVENT_COUNT_LIMIT", 1000
+)
+SPAN_LINK_COUNT_LIMIT = Configuration().get(
+    "SPAN_LINK_COUNT_LIMIT", 1000
+)

It makes the feature clunky to test, I don't feel comfortable shipping that but maybe I simply did not understand your suggestion 😐.

I believe that a way to implement those limitations in the code without violating the API is to use intermediary classes. That allows us to leverage the TracerProvider, as @toumorokoshi suggested. According to the spec, I understand that this is where this kind of configuration should live:

In implementations of the API, the TracerProvider is expected to be the stateful object that holds any configuration.

I've updated the PR with that, tell me what you think about it. The style does not pass, but I'd really like a feedback before investing more time in either fixing the style or finding a better way to implement the "global" solution

lzchen · 2020-11-30T15:58:30Z

@LetzNico
Your original code of:

-MAX_NUM_ATTRIBUTES = 1000
-MAX_NUM_EVENTS = 1000
-MAX_NUM_LINKS = 1000
+SPAN_ATTRIBUTE_COUNT_LIMIT = Configuration().get(
+    "SPAN_ATTRIBUTE_COUNT_LIMIT", 1000
+)
+SPAN_EVENT_COUNT_LIMIT = Configuration().get(
+    "SPAN_EVENT_COUNT_LIMIT", 1000
+)
+SPAN_LINK_COUNT_LIMIT = Configuration().get(
+    "SPAN_LINK_COUNT_LIMIT", 1000
+)

is what I was referring to and is correct.

If you need guidance on how to test this, you can patch the the environment variables and pass in your own values like here.

This adds support for the following env variables: * OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT (Maximum allowed span attribute count) * OTEL_SPAN_EVENT_COUNT_LIMIT (Maximum allowed span event count) * OTEL_SPAN_LINK_COUNT_LIMIT (Maximum allowed span link count)

Some of its tests set env vars so they can be retrieved in the Configuration; since some modules initialize it at load-time, it needs to be reset before and after tests

LetzNico · 2020-12-05T13:41:04Z

I think we're good now @lzchen ! All of this time for... just that, sorry for the time it took :)

…-vars

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch 3 times, most recently from 90a5f86 to 9def1dc Compare November 15, 2020 23:39

LetzNico marked this pull request as ready for review November 15, 2020 23:46

LetzNico requested review from a team, toumorokoshi and lzchen and removed request for a team November 15, 2020 23:46

lzchen reviewed Nov 16, 2020

View reviewed changes

toumorokoshi suggested changes Nov 17, 2020

View reviewed changes

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from 9def1dc to b7f367e Compare November 21, 2020 23:31

LetzNico requested review from toumorokoshi and lzchen November 21, 2020 23:39

toumorokoshi approved these changes Nov 24, 2020

View reviewed changes

toumorokoshi reviewed Nov 24, 2020

View reviewed changes

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from b7f367e to 110011f Compare November 29, 2020 17:24

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from 110011f to 894e4a7 Compare December 4, 2020 16:20

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from 894e4a7 to 138cb45 Compare December 4, 2020 16:36

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from 138cb45 to fd460de Compare December 4, 2020 17:08

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from fd460de to 7096de6 Compare December 4, 2020 17:13

Reset config before/after jaeger export tests

ee66977

Some of its tests set env vars so they can be retrieved in the Configuration; since some modules initialize it at load-time, it needs to be reset before and after tests

LetzNico force-pushed the 1364-support-span-collection-limit-via-env-vars branch from d4ef006 to ee66977 Compare December 5, 2020 13:22

Merge branch 'master' into 1364-support-span-collection-limit-via-env…

ef750b9

…-vars

lzchen approved these changes Dec 7, 2020

View reviewed changes

Merge branch 'master' into 1364-support-span-collection-limit-via-env…

96cea06

…-vars

lzchen merged commit 553bdfc into open-telemetry:master Dec 8, 2020

codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Dec 8, 2020

Add support for span collection limit via env vars (open-telemetry#1377)

a7b2b92

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for span collection limit via env vars #1377

Add support for span collection limit via env vars #1377

LetzNico commented Nov 12, 2020 •

edited by lzchen

Loading

LetzNico commented Nov 15, 2020

lzchen Nov 16, 2020

toumorokoshi Nov 17, 2020

lzchen Nov 17, 2020

toumorokoshi Nov 18, 2020

lzchen Nov 18, 2020

toumorokoshi left a comment

toumorokoshi Nov 17, 2020

LetzNico commented Nov 21, 2020

lzchen commented Nov 24, 2020

LetzNico commented Nov 24, 2020

lzchen commented Nov 24, 2020

lzchen commented Nov 24, 2020

toumorokoshi left a comment

toumorokoshi Nov 24, 2020

LetzNico commented Nov 29, 2020

lzchen commented Nov 30, 2020

LetzNico commented Dec 5, 2020

Add support for span collection limit via env vars #1377

Add support for span collection limit via env vars #1377

Conversation

LetzNico commented Nov 12, 2020 • edited by lzchen Loading

Description

Type of change

How Has This Been Tested?

Checklist:

LetzNico commented Nov 15, 2020

lzchen Nov 16, 2020

Choose a reason for hiding this comment

toumorokoshi Nov 17, 2020

Choose a reason for hiding this comment

lzchen Nov 17, 2020

Choose a reason for hiding this comment

toumorokoshi Nov 18, 2020

Choose a reason for hiding this comment

lzchen Nov 18, 2020

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi Nov 17, 2020

Choose a reason for hiding this comment

LetzNico commented Nov 21, 2020

lzchen commented Nov 24, 2020

LetzNico commented Nov 24, 2020

lzchen commented Nov 24, 2020

lzchen commented Nov 24, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi Nov 24, 2020

Choose a reason for hiding this comment

LetzNico commented Nov 29, 2020

lzchen commented Nov 30, 2020

LetzNico commented Dec 5, 2020

LetzNico commented Nov 12, 2020 •

edited by lzchen

Loading