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 otlp_file exporter #154

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Add otlp_file exporter #154

merged 4 commits into from
Jan 15, 2025

Conversation

brettmc
Copy link
Contributor

@brettmc brettmc commented Jan 14, 2025

@brettmc brettmc requested a review from a team as a code owner January 14, 2025 04:04
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

This makes sense to me. We're still sorting out how to handle configuration for parts of the spec that aren't stable in #142 and we'll need to adjust this new type prior to stabilizing declarative configuration, but I see no reason to not proceed with this as is for now.

otlp_file:
# Configure output stream.
# If omitted or null, stdout is used.
output_stream: logs.jsonl
Copy link
Member

Choose a reason for hiding this comment

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

If we're naming this output_stream, perhaps we should require a scheme to indicate that the stream is a file vs. something else we may support in the future. I.e. file:///logs.jsonl.

Note: 3 slashes is intentional. First two are for the scheme file://, third is for the absolute path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, and that does allow for future expansion. I'll document and add examples for file://. I think that stdout should be a special case, as it seems simpler than file:///dev/stdout

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, see optional suggestions.

otlp_file:
# Configure output stream.
# If omitted or null, stdout is used.
output_stream: metrics.jsonl
Copy link
Member

Choose a reason for hiding this comment

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

For completeness of the example, add temporality_preference and default_histogram_aggregation ?

This is the kitchen sink, it should show all settings.

document and add examples for file:// and stdout methods of defining output_stream
expand kitchen-sink examples
- type: OtlpFileExporter
property_descriptions:
output_stream: >
Configure output stream. Values include stdout, or scheme+destination. For example: file:///path/to/file.jsonl.
Copy link
Member

Choose a reason for hiding this comment

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

#nit: In #151 as a result of feedback about long lines in field descriptions, I adopted a pattern where:

  • First line is a terse description of what the field is.
  • Details about semantics of the value are broken out on to a separate line.
  • Details about default / unset semantics are broken out on to a separate line.
Suggested change
Configure output stream. Values include stdout, or scheme+destination. For example: file:///path/to/file.jsonl.
Configure output stream.
Values include stdout, or scheme+destination. For example: file:///path/to/file.jsonl.

Copy link
Member

Choose a reason for hiding this comment

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

I figure you're offline right now, so I went ahead and pushed a commit to reflect this recommendation to merge. Hope that's ok. 🙂

@jack-berg jack-berg merged commit 30fb26e into open-telemetry:main Jan 15, 2025
11 checks passed
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.

3 participants