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

Updating ReadMEs for Jaeger, Zipkin, OTLP exporters #1591

Merged
merged 6 commits into from
Nov 19, 2020
Merged

Updating ReadMEs for Jaeger, Zipkin, OTLP exporters #1591

merged 6 commits into from
Nov 19, 2020

Conversation

Austin-Tan
Copy link
Member

One thought: Does the ReadMe look too cluttered this way? We are listing every option available in configuring the exporter. The descriptions are brief, but it is still a bullet point of 5-ish entries each. @reyang @cijothomas @eddynaka please advise.

Changes

#1584 #1587 #1504 all added the batch exporter and exporter type properties. We include these in the ReadMEs.

#1557 and #1572 removed the ServiceName option from Otlp and Jaeger Exporters. We removed these from ReadMes.

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

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

@Austin-Tan Austin-Tan requested a review from a team November 19, 2020 03:55
* `Endpoint`: Target to which the exporter is going to send traces or metrics.
* `Credentials`: Client-side channel credentials.
* `Headers`: Optional headers for the connection.
* `ChannelOptions`: gRPC channel options.
* `ExportProcessorType`: Whether the exporter should use
[Batch or Simple base exporter](https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry#introduction)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I linked to the API intro where it mentioned those but this is a better, more guided and specific reference. Thanks I'll update it.

Copy link
Member

Choose a reason for hiding this comment

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

Most of the time, linking to spec is right, as its our source of truth! For anything specific to .NET implementation (this repo), we should link to this repo itself.

@cijothomas
Copy link
Member

One thought: Does the ReadMe look too cluttered this way? We are listing every option available in configuring the exporter. The descriptions are brief, but it is still a bullet point of 5-ish entries each.

Agree. We can have docs explain the concepts, and also explain options which requires some explanation. And then simply link to the actual class file for the full list of option. (Lets address this separately, as we need to do it for pretty much all instrumentation)

@cijothomas cijothomas merged commit 5066600 into open-telemetry:master Nov 19, 2020
@Austin-Tan Austin-Tan deleted the UpdateReadMesBatchOptions branch November 19, 2020 23:00
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.

2 participants