feat: add max_attributes_per_event and max_attributes_per_link. #521

TommyCpp · 2021-04-15T00:30:32Z

Added max_attributes_per_event and max_attributes_per_link.

We enforced those rules in tracer or spans rather than having an EvictedQueue is because we cannot get the max_attributes when we create links or events. We cannot assume the users are using a tracer created by the global tracer provider as in theory users can have multiple tracer providers.

Need to add unit tests

codecov · 2021-04-15T00:41:36Z

Codecov Report

Merging #521 (90df68a) into main (7e88009) will increase coverage by 1.1%.
The diff coverage is 71.4%.

@@           Coverage Diff           @@
##            main    #521     +/-   ##
=======================================
+ Coverage   51.1%   52.3%   +1.1%     
=======================================
  Files         95      96      +1     
  Lines       8395    8503    +108     
=======================================
+ Hits        4296    4448    +152     
+ Misses      4099    4055     -44

Impacted Files	Coverage Δ
opentelemetry/src/sdk/trace/config.rs	`29.0% <5.0%> (-5.0%)`	⬇️
opentelemetry/src/sdk/trace/span_limit.rs	`33.3% <33.3%> (ø)`
opentelemetry/src/sdk/trace/tracer.rs	`85.7% <56.5%> (+0.7%)`	⬆️
opentelemetry/src/sdk/trace/span.rs	`95.0% <98.5%> (+8.9%)`	⬆️
opentelemetry/src/testing/trace.rs	`63.4% <100.0%> (ø)`
opentelemetry/src/trace/link.rs	`73.6% <100.0%> (+73.6%)`	⬆️
opentelemetry/src/sdk/trace/span_processor.rs	`81.4% <0.0%> (+0.7%)`	⬆️
opentelemetry/src/trace/noop.rs	`74.2% <0.0%> (+2.9%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e88009...90df68a. Read the comment docs.

jtescher · 2021-04-15T01:31:02Z

Nice to add the additional limit config 👍, I wonder if there may be a better way to enforce at event creation (or if we need to change the event creation api to support limits better)

TommyCpp · 2021-04-15T01:33:48Z

I wonder if there may be a better way to enforce at event creation (or if we need to change the event creation api to support limits better)

I do think we should make members of Event private just in case we need to add fields.

But in order to enforce the rules. Event must have some connection with the tracer provider when creating because the limit was stored in the tracer provider. We could use a tracer to create an event but I don't think it's in the spec and it feels wired TBH.

opentelemetry/src/sdk/trace/span.rs

jtescher · 2021-04-17T15:49:51Z

@TommyCpp looks like the current config struct was renamed to span limits in open-telemetry/opentelemetry-specification#1416, what do you think about the idea of having spans storing their limits directly to avoid the tracer provider lookup (e.g. the go version)

TommyCpp · 2021-04-17T18:26:46Z

what do you think about the idea of having spans storing their limits directly to avoid the tracer provider lookup (e.g. the go version)

We could do that, it increases the size of a span a little but we can avoid upgrading weak pointer in the tracer. But we still need to enforce the limit when adding events or links.

jtescher · 2021-04-17T20:25:40Z

opentelemetry/src/sdk/trace/span_limit.rs

+
+/// Span limit configuration to keep attributes, events and links to a span in a reasonable number.
+#[derive(Copy, Clone, Debug)]
+pub struct SpanLimit {


I think the spec wants this pluralized

Suggested change

pub struct SpanLimit {

pub struct SpanLimits {

jtescher · 2021-04-17T20:31:35Z

opentelemetry/src/sdk/trace/tracer.rs

            if let Some(link_options) = &mut link_options {
+                for link in link_options.iter_mut() {


Do we still need to do this? going through append_vec below will call push_back which should already handle the limits configured above

If I understand correctly, the append_vec only enforces the limit on the number of links. Not the number of attributes of a link

Oh sorry I see what's happening nvm

It may make sense to redesign how we create a link to enforce the limit when we creating one 🤔 in the future. Gonna have to use EvictedQueue to store the attributes too

Yeah can rethink he we store dropped count and others in a follow up PR perhaps

My current plan is to add fields in SpanLimits to allow us to store the dropped number in it.
Also need to print the warning message when the dropped number reaches some threshold. Cannot really warn whenever we drop because the spec says don't be too noisy on this issue.

jtescher · 2021-04-17T20:59:29Z

opentelemetry/src/sdk/trace/span.rs

@@ -51,11 +53,19 @@ impl Span {
        span_context: SpanContext,
        data: Option<SpanData>,
        tracer: sdk::trace::Tracer,
+        span_limit: Option<SpanLimits>,


Does this need to be optional? Could switch tests to use Default::default() instead of None

If the span_limit is None. Then we will try to get the span limit from the tracer provider. If there isn't a tracer provider connected with the tracer, we will use the default.

Sometimes we may change the span_limit during the span building(For example, we may record the number of dropped entities in the future) so we may pass modified span_limit. But we want to keep the span limit the same as the tracer provider, we can pass None.

Hm yeah we should store them somewhere. Maybe best path is to have SpanLimits not be optional here and add dropped counts to SpanData? keeps things simple and tracks attribute drops without having to upgrade the provider.

Sounds reasonable. I removed the option around SpanLimits. I probably would open another PR to address how to record and expose the number of dropped entities.

jtescher

Ok good place to start

TommyCpp requested a review from a team as a code owner April 15, 2021 00:30

TommyCpp force-pushed the tommycpp/span_limit branch from ad9a7e3 to 3bb440b Compare April 15, 2021 00:37

feat: add max_attributes_per_event and max_attributes_per_link.

d73a4b5

TommyCpp force-pushed the tommycpp/span_limit branch from 3bb440b to d73a4b5 Compare April 15, 2021 00:42

TommyCpp added 2 commits April 15, 2021 21:20

fix: bugs, add unit tests.

5269e8a

fix: format

816f1d7

frigus02 reviewed Apr 16, 2021

View reviewed changes

opentelemetry/src/sdk/trace/span.rs Outdated Show resolved Hide resolved

TommyCpp added 2 commits April 17, 2021 14:27

fix: use truncate instead of drain.

f409212

feat: add span limit.

c58a9d6

jtescher reviewed Apr 17, 2021

View reviewed changes

fix: rename SpanLimit to SpanLimits

d3fa480

jtescher reviewed Apr 17, 2021

View reviewed changes

TommyCpp and others added 2 commits April 20, 2021 20:10

fix: make span limits non optional.

be8636c

Merge branch 'main' into tommycpp/span_limit

90df68a

jtescher approved these changes Apr 21, 2021

View reviewed changes

jtescher merged commit d3a9dfb into open-telemetry:main Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add max_attributes_per_event and max_attributes_per_link. #521

feat: add max_attributes_per_event and max_attributes_per_link. #521

TommyCpp commented Apr 15, 2021

codecov bot commented Apr 15, 2021 •

edited

jtescher commented Apr 15, 2021

TommyCpp commented Apr 15, 2021 •

edited

jtescher commented Apr 17, 2021

TommyCpp commented Apr 17, 2021

jtescher Apr 17, 2021

jtescher Apr 17, 2021

TommyCpp Apr 17, 2021 •

edited

jtescher Apr 17, 2021

TommyCpp Apr 17, 2021

jtescher Apr 17, 2021

TommyCpp Apr 17, 2021

jtescher Apr 17, 2021

TommyCpp Apr 17, 2021 •

edited

jtescher Apr 20, 2021

TommyCpp Apr 21, 2021

jtescher left a comment

		if let Some(link_options) = &mut link_options {
		for link in link_options.iter_mut() {

feat: add max_attributes_per_event and max_attributes_per_link. #521

feat: add max_attributes_per_event and max_attributes_per_link. #521

Conversation

TommyCpp commented Apr 15, 2021

codecov bot commented Apr 15, 2021 • edited

Codecov Report

jtescher commented Apr 15, 2021

TommyCpp commented Apr 15, 2021 • edited

jtescher commented Apr 17, 2021

TommyCpp commented Apr 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyCpp Apr 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyCpp Apr 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtescher left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 15, 2021 •

edited

TommyCpp commented Apr 15, 2021 •

edited

TommyCpp Apr 17, 2021 •

edited

TommyCpp Apr 17, 2021 •

edited