Propagate resource to exporters #706

lalitb · 2021-04-27T14:38:03Z

Fixes #334

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2021-04-27T14:57:34Z

Codecov Report

Merging #706 (88d914e) into main (fdef43b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #706      +/-   ##
==========================================
+ Coverage   94.72%   94.77%   +0.04%     
==========================================
  Files         216      217       +1     
  Lines        9900     9928      +28     
==========================================
+ Hits         9378     9409      +31     
+ Misses        522      519       -3

Impacted Files	Coverage Δ
sdk/include/opentelemetry/sdk/trace/recordable.h	`100.00% <ø> (ø)`
...de/opentelemetry/ext/zpages/threadsafe_span_data.h	`97.40% <100.00%> (+0.03%)`	⬆️
...include/opentelemetry/sdk/trace/multi_recordable.h	`94.00% <100.00%> (+0.38%)`	⬆️
sdk/include/opentelemetry/sdk/trace/span_data.h	`100.00% <100.00%> (ø)`
sdk/include/opentelemetry/sdk/trace/tracer.h	`100.00% <100.00%> (ø)`
sdk/src/trace/span.cc	`89.13% <100.00%> (+0.11%)`	⬆️
sdk/test/trace/span_data_test.cc	`100.00% <100.00%> (ø)`
...pi/include/opentelemetry/nostd/detail/functional.h	`100.00% <0.00%> (ø)`
api/include/opentelemetry/nostd/mpark/variant.h	`97.35% <0.00%> (+0.16%)`	⬆️
sdk/src/logs/batch_log_processor.cc	`95.00% <0.00%> (+1.25%)`	⬆️
... and 1 more

jsuereth · 2021-04-28T17:26:21Z

sdk/include/opentelemetry/sdk/trace/span_data.h

@@ -244,6 +259,7 @@ class SpanData final : public Recordable
  std::vector<SpanDataEvent> events_;
  std::vector<SpanDataLink> links_;
  opentelemetry::trace::SpanKind span_kind_{opentelemetry::trace::SpanKind::kInternal};
+  opentelemetry::sdk::resource::ResourceAttributes resourceAttributes_;


What is this used for? Is it worth copying the resource attributes in every span?

Thanks for noticing it. Will make it reference/pointer to resource :)

jsuereth · 2021-04-28T17:27:23Z

Generally, LGTM.

maxgolov · 2021-04-29T04:47:14Z

exporters/zipkin/include/opentelemetry/exporters/zipkin/recordable.h

@@ -67,6 +71,7 @@ class Recordable final : public sdk::trace::Recordable

 private:
  ZipkinSpan span_;
+  std::string service_name_;


I'm thinking here about some optimization opportunity: how often do you expect service_name value to change, i.e. do you have to copy it here for every recordable, then again into every JSON (ZipkinSpan) object?

We can think of some optimization of not passing resource and instrumentation library as part of every Recordable( and instead just passing tracer reference from where we can get both), but still, these data need to be copied to every ZipkinSpan. ( https://zipkin.io/zipkin-api/#/default/post_spans ).

That's fine for now. I am looking at Zipkin exporter as example for Fluentd exporter, I can share some thoughts how I would've rearranged this in the Fluentd exporter.

Thanks, will look forward to that :)

maxgolov · 2021-04-29T04:48:37Z

exporters/zipkin/src/recordable.cc

@@ -213,6 +213,16 @@ void Recordable::SetName(nostd::string_view name) noexcept
  span_["name"] = name.data();
 }

+void Recordable::SetResource(const opentelemetry::sdk::resource::Resource &resource) noexcept


Confirming re. thread-safety of SetResource vs GetServiceName -- both cannot be called at once from two different threads?

SetResource would be called when Span is created using Tracer::StartSpan(). And GetServiceName() would be when Span is ending : Span::End() -> Processor::OnEnd() -> Exporter::Export(). Both these need to be sequential in same thread.

I would let it go for now. But we may need to describe some of these expectations where we do not explicitly shield the access with a mutex, esp. in the other place where we return an object (that may get changed in another thread) by reference.

Good point. Specifically for Resources, these are immutable as per the specs, with their ownership transferred to TracerProvider while it's initialization. So there won't be race condition arising while accessing them within the pipeline.

maxgolov · 2021-04-29T04:53:14Z

sdk/include/opentelemetry/sdk/trace/tracer.h

@@ -50,6 +50,9 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th
    return *instrumentation_library_;
  }

+  /** Returns the currently configured resource **/
+  const opentelemetry::sdk::resource::Resource &GetResource() { return context_->GetResource(); }


No setter - SetResource in API, only getter - GetResource ? What are the thread-safety guarantees of this method on API surface?

The resource is a property of TracerProvider and should be set during the initialization of TracerProvider in the main thread. It's not re-settable again once initialized.

I approved.

From a practical standpoint, however, we do have scenarios as follows:

whatever Monitoring Agent is upgraded, OR remote configuration is upgraded; so your resource values changed...

your resource contents now may need to change, e.g. updated configuration of the TracerProvider - to indicate that a new configuration is now applied. Without application service restart. Your threads continue running, while accessing resource, while the resource object is maybe changing at the same time.. which may lead to crash.

In those cases, I think, we may need to consider how to make some of this thread-safe.

Current logic of returning Resource by reference in SDK may break if Resource needs to change in a different thread. I am not sure how to track my concern. Maybe we need to have a generic umbrella issue that captures thread-safety guarantees at both levels?

API thread safety guarantees

SDK thread safety guarantees

In a separate markdown document, dev-focused document for exporter developers.. Or when you return a reference, you can check where else that object may be concurrently modified, and explain why it should not be concurrently modified elsewhere. Such as explicitly spell out the thread-safety guarantees on that object that you returned by reference.

your resource contents now may need to change, e.g. updated configuration of the TracerProvider - to indicate that a new configuration is now applied. Without application service restart. Your threads continue running, while accessing resource, while the resource object is maybe changing at the same time.. which may lead to crash.

The ownership model of the Resource object is well defined. Once created, the ownership of the Resource object is transferred to TracerProvider, and there is no setters to update it at runtime.

Also to add, as per the specs : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md

When used with distributed tracing, a resource can be associated with the TracerProvider when the TracerProvider is created. That association cannot be changed later. When associated with a TracerProvider, all Spans produced by any Tracer from the provider MUST be associated with this Resource.

The resource once set can't be changed later.

exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h

exporters/otlp/src/recordable.cc

sdk/src/trace/span.cc

maxgolov · 2021-05-10T05:36:19Z

sdk/include/opentelemetry/sdk/trace/span_data.h

@@ -225,6 +232,11 @@ class SpanData final : public Recordable
    span_kind_ = span_kind;
  }

+  void SetResource(const opentelemetry::sdk::resource::Resource &resource) noexcept override
+  {
+    resource_ = &resource;


@lalitb - this is problematic spot. It won't work too well with temporaries because it's assigning a pointer to reference. If Resource is compiled with std::variant, and a temporary is passed to API, e.g. SetResource("key", "value") - it'd break..

lalitb added 4 commits April 27, 2021 03:49

draft

062bc09

more changes

44e8f05

format

d7d7f5a

fix bazel

c2307ba

lalitb requested a review from a team April 27, 2021 14:38

Merge branch 'main' into resource-propagate

c81c3b6

lalitb added 5 commits April 28, 2021 04:47

fix span_data

8477133

attribute constant

42dfa0e

fix otlp

dd694cd

fix reference

56dbd3c

add todo - for resource otlp

bfd1a32

jsuereth reviewed Apr 28, 2021

View reviewed changes

lalitb added 2 commits April 29, 2021 00:52

Merge branch 'main' into resource-propagate

2f02e17

fix format

420113e

lalitb changed the title ~~[WIP] Propagate resource to exporters~~ Propagate resource to exporters Apr 28, 2021

Merge branch 'main' into resource-propagate

e92623f

maxgolov reviewed Apr 29, 2021

View reviewed changes

review comment

47f98fe

maxgolov reviewed Apr 29, 2021

View reviewed changes

review comment

e8ff60f

maxgolov approved these changes Apr 29, 2021

View reviewed changes

maxgolov and others added 2 commits April 29, 2021 15:26

Merge branch 'main' into resource-propagate

c83f3ee

Fix merge conflict

8875ce3

ThomsonTan reviewed Apr 30, 2021

View reviewed changes

exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h Outdated Show resolved Hide resolved

ThomsonTan reviewed Apr 30, 2021

View reviewed changes

exporters/otlp/src/recordable.cc Outdated Show resolved Hide resolved

sdk/src/trace/span.cc Outdated Show resolved Hide resolved

ThomsonTan approved these changes Apr 30, 2021

View reviewed changes

Trigger Build

a5d4f13

Merge branch 'main' into resource-propagate

88d914e

lalitb merged commit c7f49de into open-telemetry:main Apr 30, 2021

maxgolov reviewed May 10, 2021

View reviewed changes

maxgolov mentioned this pull request May 10, 2021

Avoid pointer to reference to temporaries in SetResource API #733

Closed

ThomsonTan mentioned this pull request May 15, 2021

Resource is not populated to OTLP proto data #757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate resource to exporters #706

Propagate resource to exporters #706

lalitb commented Apr 27, 2021

codecov bot commented Apr 27, 2021 •

edited

Loading

jsuereth Apr 28, 2021

lalitb Apr 28, 2021

jsuereth commented Apr 28, 2021

maxgolov Apr 29, 2021

lalitb Apr 29, 2021 •

edited

Loading

maxgolov Apr 29, 2021

lalitb Apr 30, 2021

maxgolov Apr 29, 2021

lalitb Apr 29, 2021 •

edited

Loading

maxgolov Apr 29, 2021

lalitb Apr 30, 2021

maxgolov Apr 29, 2021

lalitb Apr 29, 2021

maxgolov Apr 29, 2021 •

edited

Loading

lalitb Apr 30, 2021

lalitb Apr 30, 2021

maxgolov May 10, 2021

Propagate resource to exporters #706

Propagate resource to exporters #706

Conversation

lalitb commented Apr 27, 2021

Changes

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuereth commented Apr 28, 2021

Choose a reason for hiding this comment

lalitb Apr 29, 2021 • edited Loading

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

lalitb Apr 29, 2021 • edited Loading

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

maxgolov Apr 29, 2021 • edited Loading

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

codecov bot commented Apr 27, 2021 •

edited

Loading

lalitb Apr 29, 2021 •

edited

Loading

lalitb Apr 29, 2021 •

edited

Loading

maxgolov Apr 29, 2021 •

edited

Loading