Comments (12)
Related to #148
from opentelemetry-proto.
I very much like proposal 2 as it allows simplified SDKs which just send everything as-is as well as optimized upload. I would extend it however to not only common attributes, but also common resources. Like mentioned here: #94 (comment)
from opentelemetry-proto.
Common resources are already possible in the protocol. A list of spans is currently placed under a ResourceSpans
which is associated with a single Resource
. That single resource is common for all spans in that list.
from opentelemetry-proto.
The InstrumentationLibrary has a name and a version. It is not clear if
these fields are part of Resource identity or are attributes of a Span.
Presumably they should be interpreted as attributes of the Span.
This is exactly the point is a separate thing, and not an attribute or a resource, it is a different scope inside the Resource.
from opentelemetry-proto.
Common resources are already possible in the protocol. A list of spans is currently placed under a > ResourceSpans which is associated with a single Resource. That single resource is common for all > spans in that list.
The main point of InstrumentationLibrary type is that in most cases every span will have the same Resource, but library fields which used to be on Resource object as attributes were different. Thus the idea was to keep common Resource and add another level of commonality via InstrumentationLibrary.
There are cases though when a single process will host multiple apps. For this scenario having another level of resource span batch may be very efficient.
from opentelemetry-proto.
This is exactly the point is a separate thing, and not an attribute or a resource, it is a different scope inside the Resource.
@bogdandrutu Sorry, this is the part I am struggling to understand.
I understand that InstrumentationLibrary has conceptual counterparts in the API because it makes API nicer to use (getting named Tracer and using it). However why do we need to preserve this concept on the wire? Why can it not be just an attribute when it leaves the SDK?
We actually have to convert it to an attribute when when SDK is configured to use Jaeger, Zipkin or any other exporter. We have to do it because none of trace protocols has the InstrumentationLibrary concept at the protocol level. Neither does any backend have such concept.
If this is true I am failing to see why InstrumentationLibrary deserves a dedicated place in OpenTelemetry protocol.
Let me quote from protocol Design Goals document:
Be suitable for use between all of the following node types: instrumented applications, telemetry backends, local agents, stand-alone collectors/forwarders.
I find that the current design that uses InstrumentationLibrary is skewed towards the needs of OpenTelemetry SDK at the cost of making the life of other node types more complicated. None of the other nodes benefit from the existence of InstrumentationLibrary, it just adds more complexity, which is avoidable.
I see the evidence of that complexity in all translation functions in the Collector. And we even have to add a separate processor type to handle filtering by instrumentation library as shown by this newly submitted PR which would be unnecessary if instrumentation library was a regular attribute.
I agree with you that in OpenTelemetry API/SDK it is a different concept but I think that it would be nice to contain that difference in the boundaries of API/SDK and not expose all other components of collection pipeline to this concept and force them to understand and handle it.
Please let me know what you think, I may be missing something important. Perhaps worth discussing this live. :-)
from opentelemetry-proto.
@tigrannajaryan I fully support your position. I saw this concept an optimization layer and I like how this optimization can be replaced by span batches (your proposal 2). The only suggestion is to include resource attributes into SpanBatch
as well. We can also decide to implement span batches later if needed.
from opentelemetry-proto.
The only suggestion is to include resource attributes into SpanBatch as well.
I am sorry but I think this removes the entire benefit of having a Resource
. The whole idea is that there is a scope associated with these attributes, by having everything as attribute we lose an extremely important property related to the scope of the attributes:
- The resource attributes are attributes that describe the application/process source of the telemetry.
- The InstrumentationLibrary describes the
component
inside the application/process that records (which I think a lot of people have a confusion about, why this does not describe the component that produces the telemetry) the telemetry. - Span attributes are attributes that describe that are specific to that operation/span.
By merging all of them you lose a very important property of all these attributes, which will lead in lacking the ability to correlate telemetry between Resources for example.
I am a BIG -1 on removing the Resource from the protocol or adding extra informations to the Resource that do not describe the application/process that produces the telemetry.
I see the evidence of that complexity in all translation functions in the Collector. And we even have to add a separate processor type to handle filtering by instrumentation library as shown by this newly submitted PR which would be unnecessary if instrumentation library was a regular attribute.
That is not a new processor type, is extending the functionality for an already existing processor.
from opentelemetry-proto.
@bogdandrutu I'm not suggesting to remove resources. Sorry if I wasn't clear. I suggest that we extend SpanBatch from proposal 2 to also include additional resource attributes that are common to all spans in the batch. Like this:
resource_spans:
resource:
...
span_batches:
- common_attributes:
- key: instrumentation.library.name
value: io.opentelemetry.redis
- common_resource_attributes:
- key: tenant
- value: A
spans:
- name: request
start_time: 123
- common_attributes:
- key: instrumentation.library.name
value: io.opentelemetry.apache.httpd
- common_resource_attributes:
- key: tenant
- value: B
spans:
- name: request
start_time: 456
from opentelemetry-proto.
I put a bit more though into this and I am inclining towards the simplicity of Proposal 1. Unfortunately Proposal 2 creates problems of its own.
For example in Collector we have a processor which can update the value of a Span attribute if it exists. This is a one-line "update" operation if there is a single attribute map in the Span.
If we introduce the common_attributes this operation becomes a lot more complicated:
if common_attributes contains the key {
delete common_attributes[key]
insert span.attributes[key]
} else {
update span.attributes[key]
}
This appears simple but have lots of logic of this sort in Collector, some written by outside contributors who are not necessarily experts in the codebase. It will be a proliferation of checks and if you forget branch you will have hard-to-catch subtle bugs.
I did benchmarking of the "Proposal 1" and it is about 6% slower than current schema that uses InstrumentationLibrary if you actually include "instrumentation.library.name" attribute in every span (assuming 10 spans per instrumentation library). "Proposal 2" has about the same speed as current schema.
As much as I wish for the highest performance in our protocol I do not think "Proposal 2" is worth the complexity and we should use simple approach in "Proposal 1". It is easy to understand, easy to work with and difficult to make mistakes. I believe it is the right tradeoff.
I am going to look into a bit more details of this and will add a comment if I find something important.
from opentelemetry-proto.
Another issue that needs to be addressed when doing either Proposal 1 or 2 is what happens if the SDK sets "instrumentation.library.name" attribute and the user calls Span.SetAttribute API with the same key.
I believe the answer is that the behavior should be similar to what happens if the user calls Span.SetAttribute API twice with any other key. Spec defines that the second call overwrites the existing value. We want the same behavior here, user supplied "instrumentation.library.name" attribute value will prevail, which is reasonable and non-surprising behavior.
This behavior is easy to achieve in Proposal 1. SDK just needs to add "instrumentation.library.name" attribute to newly created Span data. I checked Go and Java SDK's, they both use maps for attributes, which makes this trivially achievable.
Achieving the same behavior is more complicated in Proposal 2, there is no such trivial way anymore due to the need to keep common attributes in addition to regular attributes, which is another argument in favour of Proposal 1.
I am going to submit a PR with Proposal 1 implemented. If we feel strongly about the need to have Proposal 2 done, it can be a follow up PR.
from opentelemetry-proto.
Closing, the decision is to keep InstrumentationLibrary.
from opentelemetry-proto.
Related Issues (20)
- Enum element naming consistency HOT 1
- Conflicting requirements in OTLP JSON field names HOT 14
- Schema support for span with key-value attributes HOT 2
- Question about ResourceSpans HOT 1
- Mention 1.0.0 in CHANGELOG.md? HOT 3
- Difference between proto and SDK HOT 2
- Why TraceID and SpanID are defined []byte type? HOT 4
- AnyValue diverging from spec on website HOT 2
- The comment of schema_url doesn't describe it's purpose.
- Clarify whether OTLP/gRPC codes should cover both client and server HOT 1
- Unclear whether Span.flags represents span's parent or own flags HOT 10
- Fix releasing notes HOT 3
- Clarify meaning of *DNS name prefix* in protobuf code comment for metric name
- Allow consumers to use map<string, AnyValue> instead of repeated KeyValue HOT 3
- Generated OpenAPI files wrongly encode enums
- Improve metrics.proto by mentioning Resource Attributes and Scope HOT 1
- How to use Java service to receive data in the OpenTelemetry (Otel) protocol? HOT 4
- Supporting JSON as-is encoding in OTLP HOT 2
- SPAN_FLAGS_TRACE_FLAGS_MASK is not defined HOT 3
- Fail to push limit HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from opentelemetry-proto.