-
Notifications
You must be signed in to change notification settings - Fork 773
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
[Otlp] Add disk retry enablement #5527
Changes from 10 commits
1f5a7a8
8981aca
82464dd
526e290
97eaacc
0e6734c
ee854f3
847d228
3417006
edf649e
1371484
634a2be
d41f14d
d4eab4d
44b7bf0
053d1bb
2a42d1a
a909b96
2d7ea62
8922b5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -632,6 +632,11 @@ want to solicit feedback from the community. | |
|
||
Added in `1.8.0`. | ||
|
||
When set to `disk` along with setting | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a user, I think I would have a lot of questions about this feature. What are the file names? What is the file structure? What is the retention policy? Are there multiple files? What do these files mean? How are they managed? How are they cleaned up? Stuff like that. I think we may need more docs. Doesn't have to be on this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, in addition, who has access to these files, any security/privacy concern, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree we need to explain these. I think it would be better to improve the doc here to cover these details: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.PersistentStorage.FileSystem. We could then link from here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code for persistent storage is vendored in this exporter, and it does not use a persistent storage package reference. It’s appropriate to document this here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a process defined: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/PersistentStorage/README.md#persistent-storage-apis-for-otlp-exporter. Currently we are not forking Readmes, but I think we should do that since we do not make any customizations to the forked files. |
||
`OTEL_DOTNET_EXPERIMENTAL_OTLP_DISK_RETRY_DIRECTORY_PATH` to the path on | ||
disk, it enables retries by storing telemetry on disk during transient | ||
errors. TODO: Add package version. | ||
vishweshbankwar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
* Logs | ||
|
||
* `OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an app is designed to run in a platform-independent environment and wants to use a temporary location, the restriction to set the environment variable
OTEL_DOTNET_EXPERIMENTAL_OTLP_DISK_RETRY_DIRECTORY_PATH
can make things complex. Should we write to the temporary path if this environment variable is not set? We could rely on .NET API to get temp pathPath.GetTempPath
. Customers will use OTEL_DOTNET_EXPERIMENTAL_OTLP_RETRY to enable offline storage anyway. We could explain the behavior in the documentation along with this environment variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. Its more simple for users who want minimal configuration for trying out this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated