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

Fork in persistent storage implementation to otlp #4793

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Aug 21, 2023

Towards #4791
Design discussion issue #

Changes

This PR forks in OpenTelemetry.PersistentStorage.Abstractions and OpenTelemetry.PersistentStorage.FileSystem into the otlp exporter package. The APIs from this package will be used for adding retry support via persistent storage.

The code is copied as is. It defines new constant for enabling as is copy <DefineConstants>BUILDING_INTERNAL_PERSISTENT_STORAGE</DefineConstants>

Reference PR open-telemetry/opentelemetry-dotnet-contrib#1320

Merge requirement checklist

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

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #4793 (990fddb) into main (e330e57) will decrease coverage by 2.16%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4793      +/-   ##
==========================================
- Coverage   85.59%   83.44%   -2.16%     
==========================================
  Files         284      292       +8     
  Lines       11686    11987     +301     
==========================================
- Hits        10003    10002       -1     
- Misses       1683     1985     +302     
Files Changed Coverage Δ
...Protocol/PersistentStorage/DirectorySizeTracker.cs 0.00% <0.00%> (ø)
...penTelemetryProtocol/PersistentStorage/FileBlob.cs 0.00% <0.00%> (ø)
...etryProtocol/PersistentStorage/FileBlobProvider.cs 0.00% <0.00%> (ø)
...emetryProtocol/PersistentStorage/PersistentBlob.cs 0.00% <0.00%> (ø)
...otocol/PersistentStorage/PersistentBlobProvider.cs 0.00% <0.00%> (ø)
...torage/PersistentStorageAbstractionsEventSource.cs 0.00% <0.00%> (ø)
.../PersistentStorage/PersistentStorageEventSource.cs 0.00% <0.00%> (ø)
...tocol/PersistentStorage/PersistentStorageHelper.cs 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

@vishweshbankwar vishweshbankwar marked this pull request as ready for review August 21, 2023 17:25
@vishweshbankwar vishweshbankwar requested a review from a team August 21, 2023 17:25
vishweshbankwar and others added 4 commits August 21, 2023 14:15
…rage/FileBlob.cs

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
…rage/FileBlobProvider.cs

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
@alanwest
Copy link
Member

I assume PersistentStorage will continue to be maintained in the contrib repo. If so I would expect PRs like this to be easy reviews with no alterations to the code (alterations should be done in the contrib repo) other than marking things internal, of course.

Lifting this code into the main repo is a fine strategy in the interim, though I think we need a little documentation for how we aim to maintain this lifted code. A README in the PersistentStorage directory would suffice - with links to the real source and a simple outline of steps for keeping it up to date if necessary.

@vishweshbankwar
Copy link
Member Author

I assume PersistentStorage will continue to be maintained in the contrib repo. If so I would expect PRs like this to be easy reviews with no alterations to the code (alterations should be done in the contrib repo) other than marking things internal, of course.

Lifting this code into the main repo is a fine strategy in the interim, though I think we need a little documentation for how we aim to maintain this lifted code. A README in the PersistentStorage directory would suffice - with links to the real source and a simple outline of steps for keeping it up to date if necessary.

👍 Added Readme.

…rage/README.md

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
@utpilla utpilla merged commit d0d9133 into open-telemetry:main Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants