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

feat: add max_attributes_per_event and max_attributes_per_link. #521

Merged
merged 8 commits into from
Apr 21, 2021

Conversation

TommyCpp
Copy link
Contributor

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

@TommyCpp TommyCpp requested a review from a team April 15, 2021 00:30
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Member

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
Copy link
Contributor Author

TommyCpp commented Apr 15, 2021

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.

@jtescher
Copy link
Member

@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
Copy link
Contributor Author

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.


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

Choose a reason for hiding this comment

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

I think the spec wants this pluralized

Suggested change
pub struct SpanLimit {
pub struct SpanLimits {

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@TommyCpp TommyCpp Apr 17, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry I see what's happening nvm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@TommyCpp TommyCpp Apr 17, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Ok good place to start

@jtescher jtescher merged commit d3a9dfb into open-telemetry:main Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants