Skip to content

Feature/span limits #816

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.

Sign up for GitHub

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

Closed
anshulkumar19 wants to merge 9 commits into from
Closed

Feature/span limits #816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
893aba4
adding span limits struct
anshulkumar19 May 13, 2021
249ad35
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
anshulkumar19 May 21, 2021
db79ada
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
anshulkumar19 May 31, 2021
3f14a59
span limits constraints in span data
anshulkumar19 Jun 2, 2021
d7b39fe
adding span limit to all recordables
anshulkumar19 Jun 2, 2021
179838f
Merge branch 'feature/span_limits' of https://github.com/anshulkumar1…
anshulkumar19 Jun 2, 2021
5f916bf
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
anshulkumar19 Jun 3, 2021
a35cc8c
Modifying span limits calls to use static methods
anshulkumar19 Jun 3, 2021
41c05dc
Merge branch 'main' into feature/span_limits
maxgolov Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/common/attribute_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,4 @@ class AttributeMap : public std::unordered_map<std::string, OwnedAttributeValue>
};
} // namespace common
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
OPENTELEMETRY_END_NAMESPACE
7 changes: 6 additions & 1 deletion sdk/include/opentelemetry/sdk/trace/span_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/sdk/common/attribute_utils.h"
#include "opentelemetry/sdk/trace/recordable.h"
#include "opentelemetry/sdk/trace/span_limits.h"
#include "opentelemetry/trace/canonical_code.h"
#include "opentelemetry/trace/span.h"
#include "opentelemetry/trace/span_id.h"
Expand Down Expand Up @@ -197,7 +198,11 @@ class SpanData final : public Recordable
void SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept override
{
attribute_map_.SetAttribute(key, value);
if (attribute_map_.size() < SpanLimits::GetAttributeCountLimit() ||
attribute_map_.find(std::string(key)) != attribute_map_.end())
{
attribute_map_.SetAttribute(key, value);
}
Copy link
Copy Markdown
Member

@lalitb lalitb Jun 3, 2021

Choose a reason for hiding this comment

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

Not sure if this is a good idea to drop the attributes silently.

Copy link
Copy Markdown
Contributor Author

@anshulkumar19 anshulkumar19 Jun 3, 2021

Choose a reason for hiding this comment

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

Agreed.
We can either log every time when attribute is dropped, but that would be excessive logging and is not recommended in the spec.
Second way, we can store the information of how many attributes are added in the span and so the number of attributes dropped. This should be enough information, any opinion on this?

Copy link
Copy Markdown
Contributor

@maxgolov maxgolov Jun 3, 2021

Choose a reason for hiding this comment

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

Yeah, this won't really work... Because I'd see that class being used not necessarily only for Span Attributes. But for Resource Attributes as well. Do resource attributes have the same limit? I mean - can we move this check elsewhere.. Can we keep it a generic converter-helper class to transform attributes from API AttributeValue to OwnedAttributeValue, and just keep it as generic as possible, without binding it to Span specifics.

Copy link
Copy Markdown
Member

@lalitb lalitb Jun 4, 2021

Choose a reason for hiding this comment

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

The limit is specific to Span attributes, links, and events. SDK doesn't own/copy any of these properties internally, so it is not possible to impose this limit within sdk. All these properties are propagated to exporters, and it is up to the exporters to impose the limits. The exporters may choose not to transform these properties to OwnedAttributeValue and instead transform/serialize to (say) JSON format for performance reasons.

Copy link
Copy Markdown
Contributor

@maxgolov maxgolov Jun 4, 2021

Choose a reason for hiding this comment

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

@lalitb - I think we should have that trigger (limit enforcement) somewhere when we transform into OwnedAttributeValue. I don't have concrete proposals. Maybe we should merge the foundation class for some enforcement, proposed here in this PR. Then find out a good way to bind to that class.

}

void AddEvent(nostd::string_view name,
Expand Down
119 changes: 119 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/span_limits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// Copyright 2021, OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#pragma once
#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/nostd/shared_ptr.h"
#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
/**
* Collection of span limits configurations.
*/
class SpanLimits
{
public:
/**
* Setter methods of configuration params
*/
static void SetAttributeCountLimit(int attributeCountLimit)
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
GetSpanLimits()->AttributeCountLimit = attributeCountLimit;
}

static void SetEventCountLimit(int eventCountLimit)
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
GetSpanLimits()->EventCountLimit = eventCountLimit;
}

static void SetLinkCountLimit(int linkCountLimit)
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
GetSpanLimits()->LinkCountLimit = linkCountLimit;
}

static void SetAttributePerEventCountLimit(int attributePerEventCountLimit)
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
GetSpanLimits()->AttributePerEventCountLimit = attributePerEventCountLimit;
}

static void SetAttributePerLinkCountLimit(int attributePerLinkCountLimit)
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
GetSpanLimits()->AttributePerLinkCountLimit = attributePerLinkCountLimit;
}

/**
* Getter methods of configuration params
*/
static int GetAttributeCountLimit()
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
return GetSpanLimits()->AttributeCountLimit;
}

static int GetEventCountLimit()
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
return GetSpanLimits()->EventCountLimit;
}

static int GetLinkCountLimit()
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
return GetSpanLimits()->LinkCountLimit;
}

static int GetAttributePerEventCountLimit()
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
return GetSpanLimits()->AttributePerEventCountLimit;
}

static int GetAttributePerLinkCountLimit()
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
return GetSpanLimits()->AttributePerLinkCountLimit;
}

private:
static nostd::shared_ptr<SpanLimits> &GetSpanLimits() noexcept
{
static nostd::shared_ptr<SpanLimits> spanLimits(new SpanLimits);
return spanLimits;
}

static opentelemetry::common::SpinLockMutex &GetLock() noexcept
{
static opentelemetry::common::SpinLockMutex lock;
return lock;
}

int AttributeCountLimit = 128;
Copy link
Copy Markdown
Contributor

@maxgolov maxgolov Jun 3, 2021

Choose a reason for hiding this comment

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

Sorry for naïve question :

  1. are we anticipating Span limits for different exporters (Jaeger, Zipkin, OTLP, or some other custom exporter, say FluentD-MsgPack) to be the same, and following these exact defaults?

  2. If the answer to Q1 is No, then can we have these limits make somehow tunable, and be part of some form of Exporter configuration? This is becoming a bit tricky though. In case of multi-processor, in case if we want to route our telemetry to different exporter, and if every exporter has its own limits..

Apologies if it's a dumb question.

Copy link
Copy Markdown
Contributor Author

@anshulkumar19 anshulkumar19 Jun 4, 2021

Choose a reason for hiding this comment

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

Spec mentions these limits set to tracer provider and we have one global tracer provider, so in the current scenario all of these limits should be same.
Moreover, according to my understanding since these limits are there for the safety from erroneous code adding absurd number of attributes to a span so it should be related to span related limits rather than depending upon any exporters. So, once these limits are defined they should be followed for each kind of span (sdk/exporters).

int EventCountLimit = 128;
int LinkCountLimit = 128;
int AttributePerEventCountLimit = 128;
int AttributePerLinkCountLimit = 128;
};
} // namespace trace
} // namespace sdk

Copy link
Copy Markdown
Member

@lalitb lalitb Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can this be a class with static methods to return the limits?

Copy link
Copy Markdown
Contributor Author

@anshulkumar19 anshulkumar19 Jun 3, 2021

Choose a reason for hiding this comment

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

Since we are having a global tracer provider and span limits are associated with that only, so that should be better as it will simplify arguments passing. Will make the changes in next commit.

OPENTELEMETRY_END_NAMESPACE
4 changes: 2 additions & 2 deletions sdk/src/trace/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ nostd::shared_ptr<trace_api::Span> Tracer::StartSpan(
auto span_context = std::unique_ptr<trace_api::SpanContext>(new trace_api::SpanContext(
trace_id, span_id, trace_api::TraceFlags{trace_api::TraceFlags::kIsSampled}, false,
sampling_result.trace_state ? sampling_result.trace_state
: is_parent_span_valid ? parent_context.trace_state()
: trace_api::TraceState::GetDefault()));
: is_parent_span_valid ? parent_context.trace_state()
: trace_api::TraceState::GetDefault()));

auto span = nostd::shared_ptr<trace_api::Span>{
new (std::nothrow) Span{this->shared_from_this(), name, attributes, links, options,
Expand Down