Make SetResource return status, and call it synchronously in Batch[Log|Span]Processor

[lalitb]

Changes

As discussed here

Make SetResource return status, and call it blockng in Batch[Log|Span]Processor. This makes the setting of resources during initialization deterministic.

Earlier

LogExporter::set_resource(&mut self, resource: &opentelemetry_sdk::Resource);
LogProcessor::set_resource(&self, _resource: &Resource);

SpanExporter::set_resource(&mut self, resource: &opentelemetry_sdk::Resource);
SpanProcessor::set_resource(&self, _resource: &Resource);

Now:

LogExporter::set_resource(&mut self, resource: &opentelemetry_sdk::Resource) -> LogResult<()>;
LogProcessor::set_resource(&self, _resource: &Resource) -> LogResult<()>;

SpanExporter::set_resource(&mut self, resource: &opentelemetry_sdk::Resource) -> -> ExportResult
SpanProcessor::set_resource(&self, _resource: &Resource) -> -> ExportResult

Please provide a brief description of the changes here.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 74.9%. Comparing base (ae6e2ff) to head (696ce47).

Files	Patch %	Lines
opentelemetry-sdk/src/logs/log_processor.rs	72.4%	8 Missing ⚠️
opentelemetry-sdk/src/trace/span_processor.rs	65.2%	8 Missing ⚠️
opentelemetry-otlp/src/logs.rs	0.0%	5 Missing ⚠️
opentelemetry-sdk/src/export/logs/mod.rs	0.0%	3 Missing ⚠️
opentelemetry-otlp/src/exporter/http/logs.rs	0.0%	2 Missing ⚠️
opentelemetry-otlp/src/exporter/http/trace.rs	0.0%	2 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/logs.rs	0.0%	2 Missing ⚠️
opentelemetry-stdout/src/logs/exporter.rs	0.0%	2 Missing ⚠️
opentelemetry-stdout/src/trace/exporter.rs	0.0%	2 Missing ⚠️
opentelemetry-sdk/src/logs/log_emitter.rs	66.6%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1898     +/-   ##
=======================================
- Coverage   75.0%   74.9%   -0.1%     
=======================================
  Files        122     122             
  Lines      20289   20326     +37     
=======================================
+ Hits       15221   15229      +8     
- Misses      5068    5097     +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[utpilla]

Why does set_resource need to return a Result or impose timeout limit on it? Shouldn't it always be a matter of assigning/copying a vector?

[lalitb]

Don't want to make assumptions on what the exporter does within set_resource(). The exporter may choose to do some custom serialization for resource within this method, and then use this serialized data with every export. So better to have the status.

[cijothomas]

This makes the setting of resources during initialization deterministic.

Can you elaborate more? I am not sure I follow the intention. What is broken and how is that being fixed ?

[lalitb]

Can you elaborate more? I am not sure I follow the intention. What is broken and how is that being fixed ?

This was discussed and agreed here: #1854 (comment)
To summarise:

• Nothing is broken.
• The intention is to:
  • Catch the status from the set_resource(), and ensure that error (if any) get's logged.
  • Making Exporter::set_resource() call blocking in batch-processor, and so avoid unnecessary sleeps in tests, as discussed in above link.

This is neither a priority nor a required change. It was agreed upon earlier, and I just wanted to make such interface changes before the beta release. We can even kill this PR if we now agree with earlier interface :)

[cijothomas]

Can you elaborate more? I am not sure I follow the intention. What is broken and how is that being fixed ?

This was discussed and agreed here: #1854 (comment) To summarise:

• Nothing is broken.

• The intention is to:

  • Catch the status from the set_resource(), and ensure that error (if any) get's logged.
  • Making Exporter::set_resource() call blocking in batch-processor, and so avoid unnecessary sleeps in tests, as discussed in above link.

This is neither a priority nor a required change. It was agreed upon earlier, and I just wanted to make this interface changes before the beta release. We can even kill this PR if we now agree with earlier interface :)

Nothing is broken - Thanks for the context! I agree we need a solution to avoid random sleep, but not sure if the problem needs changes beyond BatchProcessors. BatchProcessors should be able to store the Resource reliably, irrespective of the fact that it uses channels for its internal comms. Is it possible to fix BatchProcessor alone?

[lalitb]

Is it possible to fix BatchProcessor alone?

Yes, it is possible. However, Exporter::set_resource() function can do more than just copy/store resource attributes; it could allow the exporter to pre-serialize them into the required format, and use that in every export. This serialization can fail. Previously, this step would be performed in Exporter::emit(), where any errors could have been returned.

[cijothomas]

Is it possible to fix BatchProcessor alone?

Yes, it is possible. However, Exporter::set_resource() function can do more than just copy/store resource attributes; it could allow the exporter to pre-serialize them into the required format, and use that in every export. This serialization can fail. Previously, this step would be performed in Exporter::emit(), where any errors could have been returned.

You are right that exporter can do much more than just store the attributes. In fact, I think that is the right thing for exporters to do - pre-serialize once at startup those things which won't change.

What is the SDK supposed to do, if the resource serialization fails in processor/exporter? Should it keep sending normal telemetry or abort that processor? Or should we just let Exporters/Processors deal with it (like logging, or fabricating default resource etc.)

[lalitb]

What is the SDK supposed to do, if the resource serialization fails in processor/exporter?

Currently, the SDK logs the error and continues emitting telemetry to the exporter. However, if we decide later, the return status gives flexibility for LoggerProvider::builder().with_resource("...").build() to return NoopLoggerProvider if one or more exporter(s) fail to set resource.