Update extending-the-sdk Readme to call out that the exporter can not make any assumptions about the Metric batch

[TimothyMothra]

Related to #2361.

Changes

• Update extending-the-sdk Readme to call out that the exporter can not make any assumptions about the Metric batch

Please provide a brief description of the changes here.

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

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #3071 (a10c5cc) into main (3e65569) will decrease coverage by 0.05%.
The diff coverage is n/a.

❗ Current head a10c5cc differs from pull request most recent head a7a7a1d. Consider uploading reports for the commit a7a7a1d to get more accurate results

@@            Coverage Diff             @@
##             main    #3071      +/-   ##
==========================================
- Coverage   84.71%   84.65%   -0.06%     
==========================================
  Files         259      259              
  Lines        9229     9142      -87     
==========================================
- Hits         7818     7739      -79     
+ Misses       1411     1403       -8     

Impacted Files	Coverage Δ
...rter.InMemory/InMemoryExporterMetricsExtensions.cs	66.66% <ø> (ø)
src/OpenTelemetry.Api/BaseProvider.cs	66.66% <0.00%> (-33.34%)	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	36.36% <0.00%> (-22.73%)	⬇️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	35.71% <0.00%> (-14.29%)	⬇️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs	50.00% <0.00%> (-11.12%)	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	86.75% <0.00%> (-1.99%)	⬇️
src/OpenTelemetry/Metrics/Metric.cs	94.20% <0.00%> (-1.45%)	⬇️
....Jaeger/Implementation/JaegerActivityExtensions.cs	94.96% <0.00%> (-1.45%)	⬇️
.../OpenTelemetry/Metrics/MeterProviderBuilderBase.cs	88.23% <0.00%> (-1.06%)	⬇️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs	81.63% <0.00%> (-0.73%)	⬇️
... and 12 more

[reyang]

I think this line is not by design, it's a bug that we want to fix (later).

[TimothyMothra]

Making sure we're talking about the same thing...

• "MetricApi reusing Metric instances" - This is by design, correct?
• We may fix the InMemoryExporter, but after today's meeting we're leaning towards no change. I'm not sure we want to state this as a bug in this doc.

[reyang]

Why do we think users of the InMemoryExporter will have to keep in mind / note that the SDK is doing some special optimization? This statement itself sounds like a design flaw to me.

[CodeBlanch]

@cijothomas @alanwest Are we going to keep InMemoryExporter in beta/prerelease? I agree with @reyang. Before we go stable we should have a solution for this. Some kind of safe copy thing into the collection. Because if not we are taking a dependency on the internal behavior in the public API and that might prevent us from changing the internals in the future?

[cijothomas]

The fact that SDK is re-using Metric for optimization/whatever purpose is fine. But the fact that this detail is "leaking" into exporters is the issue we are discussing.

Several options before us: (In the order of my preference)

1. Do nothing for 1.2 stable release. i.e proceed with a known issue/limitation.

2. Document that Exporters cannot assume anything about the state of the telemetry it received in the Export(), once control returns from Export() method.
   The above document will be applicable to every exporters, not just InMemoryExporter.

3. Even with above document, there is no "clone" method for the Export authors. We may address this by adding new Clone()/equivalent method. Do we need to block 1.2 for this? I'd say no - this repo itself has used InMemoryExporter for testing, even with the current limitation.

4. Modify the SDK pipeline, to always provide a snapshot/copy of Metric, which is unaffected any further action in the SDK. This requires some re-architecting, and will likely affect performance negatively.

   (Not exactly same, but related issue exists in Traces also, as any other processor/exporter can modify the Activity, affecting other exporters. We have not prevented this issue, apart from documenting it. One possible approach we briefly discussed during tracing release was to modify Exporters to receive a ReadOnlyActivity which prevents it from modifying Activity...)

[TimothyMothra]

Just to confirm for next steps

• Short Term (this PR): we want to keep the change to docs/metrics/extending-the-sdk/README.md and remove other changes.
• Long Term: do a more thorough investigation of cloning exports

[cijothomas]

Short Term (this PR): we want to keep the change to docs/metrics/extending-the-sdk/README.md and remove other changes.

^ Yes.

[CodeBlanch]

Define long term 😄 I don't think we should release OpenTelemetry.Exporter.InMemory 1.2 (stable) without addressing the API we are exposing there. If we are going to keep it prerelease until then, 👍

[cijothomas]

Yea I now realized that this would be breaking change if done after stable.
So agree, we should do this before 1.2.

[cijothomas]

Do we need to block 1.2 for this? I'd say no - this repo itself has used InMemoryExporter for testing, even with the current limitation.

^ this comment of mine is incorrect, as fixing it after 1.2 is breaking change for InMemoryExporter.