Skip to content
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.

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

Add configuration of persistent storage to OTLP Exporter #4251

Closed
wants to merge 159 commits into from
Closed

Add configuration of persistent storage to OTLP Exporter #4251

wants to merge 159 commits into from

Conversation

mic-max
Copy link
Contributor

@mic-max mic-max commented Mar 2, 2023

Towards #4115

Changes

  • Adds PersistentBlobProvider to Base OTLP gRPC Export Client that does nothing currently. Added a TODO which will be updated to conditionally write to disk retryable errors to be done in the next PR.
  • Update console example program to use it
  • Note: Only adding via the extension method, not from the OtlpExporter ctor

Screenshot showing OTLP example program running and the file it creates.
image

mic-max and others added 30 commits September 23, 2021 14:10
commit 080927d
Author: Cijo Thomas <[email protected]>
Date:   Thu Sep 23 10:07:05 2021 -0700

    Changelog update for 1.2.0.alpha4 (#2407)

commit 61aaf2e
Author: Reiley Yang <[email protected]>
Date:   Thu Sep 23 09:37:18 2021 -0700

    Simplify tutorials (#2406)

commit 78baf7c
Author: Reiley Yang <[email protected]>
Date:   Thu Sep 23 08:02:09 2021 -0700

    Implement the metrics dispose/shutdown logic (#2404)

commit 256fc2d
Author: Michael Maxwell <[email protected]>
Date:   Wed Sep 22 17:35:15 2021 -0400

    Use `Stopwatch.StartNew()` (#2403)
…her.cs


yes thank you, good catch

Co-authored-by: Robert Pająk <[email protected]>
@@ -19,6 +19,7 @@
<PackageReference Include="Grpc" Version="$(GrpcPkgVer)" Condition="'$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462'" />
<PackageReference Include="Google.Protobuf" Version="$(GoogleProtobufPkgVer)" />
<PackageReference Include="Grpc.Tools" Version="$(GrpcToolsPkgVer)" PrivateAssets="all" />
<PackageReference Include="OpenTelemetry.Extensions.PersistentStorage.Abstractions" Version="$(PersistentStoragePkgVer)" />
Copy link
Member

Choose a reason for hiding this comment

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

we need to find alternate ways to achieve this. This dependency is unlikely to be accepted for this component, as we have strict 1.5 timeline (when this component need to ship stable) and cannot be blocked as this dependency is a beta component.

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to ship stable version for the abstractions before that. Just need to rename the package
#3469

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the main/core/SDK package really on the contrib package?

Copy link
Member

Choose a reason for hiding this comment

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

This was discussed in the SIG meeting yesterday, and the general consensus is not to have this dependency.
It could be a separate OTLPWithPersistence package that does this and internally using/overriding the main OTLP Exporter.
Or
OTLP Exporter could expose some general purpose callbacks which can be leveraged by the OTLPWithPersistence package.

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #4251 (095dad7) into main (8cd0ef5) will increase coverage by 1.10%.
The diff coverage is 77.77%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4251      +/-   ##
==========================================
+ Coverage   83.15%   84.26%   +1.10%     
==========================================
  Files         296      297       +1     
  Lines       11775    11818      +43     
==========================================
+ Hits         9792     9958     +166     
+ Misses       1983     1860     -123     
Impacted Files Coverage Δ
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 90.90% <50.00%> (-0.85%) ⬇️
src/OpenTelemetry/ProviderExtensions.cs 95.00% <50.00%> (-5.00%) ⬇️
...ge/OtlpTraceExporterPersistentStorageExtensions.cs 57.89% <57.89%> (ø)
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs 85.71% <100.00%> (+1.50%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 93.75% <100.00%> (+0.89%) ⬆️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 96.42% <100.00%> (+0.06%) ⬆️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs 98.41% <100.00%> (+1.74%) ⬆️
src/OpenTelemetry/Resources/ResourceBuilder.cs 91.52% <100.00%> (+1.72%) ⬆️
...enTelemetry/Resources/ResourceBuilderExtensions.cs 92.85% <100.00%> (ø)
...Telemetry/Metrics/MetricPointOptionalComponents.cs 80.00% <0.00%> (-20.00%) ⬇️
... and 12 more

@mic-max mic-max marked this pull request as ready for review March 7, 2023 00:36
@mic-max mic-max requested a review from a team March 7, 2023 00:36
@mic-max mic-max marked this pull request as draft March 7, 2023 00:36
@mic-max mic-max marked this pull request as ready for review March 7, 2023 18:36
@mic-max mic-max requested review from CodeBlanch, vishweshbankwar and cijothomas and removed request for CodeBlanch, vishweshbankwar and cijothomas March 8, 2023 20:18
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 16, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants