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

Begin implementation of OTLP/HTTP exporter #1928

Merged

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Oct 9, 2020

This adds the initial config and factory boilerplate and corresponding tests.

The next PR will add the exporting logic.

Contributes to: #882

Added README.

@tigrannajaryan tigrannajaryan changed the title This adds the initial config and factory boilerplate and correspondin… Begin implementation of OTLP/HTTP exporter Oct 9, 2020
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/otlp_http branch from b7c9c80 to 0cbf345 Compare October 9, 2020 14:56
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #1928 into master will decrease coverage by 0.14%.
The diff coverage is 67.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1928      +/-   ##
==========================================
- Coverage   91.39%   91.24%   -0.15%     
==========================================
  Files         284      286       +2     
  Lines       16740    16841     +101     
==========================================
+ Hits        15299    15367      +68     
- Misses       1009     1036      +27     
- Partials      432      438       +6     
Impacted Files Coverage Δ
exporter/otlphttpexporter/otlp.go 34.28% <34.28%> (ø)
exporter/otlphttpexporter/factory.go 84.84% <84.84%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9aa0c5e...85e217e. Read the comment docs.

- `key_file` path to the TLS key to use for TLS required connections. Should
only be used if `insecure` is set to false.
- `timeout` (default = 30s): How long to wait until the connection is close.
- `read_buffer_size` (default = 0): ReadBufferSize for HTTP client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that reads are for error reporting purposes (I'm assuming), is it really so important to be able to configure the read buffer size?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not important, but it already exists in the HTTPClientSettings. Perhaps we can keep it undocumented to avoid overloading the users with unnecessary settings?

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/otlp_http branch 3 times, most recently from 0718230 to b1c6894 Compare October 9, 2020 15:22
This adds the initial config and factory boilerplate and corresponding tests.

Contributes to: open-telemetry#882

Testing: config and factory tests unit tests added.

Documentation: Added README.

The next PR will add the exporting logic.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/otlp_http branch from b1c6894 to 85e217e Compare October 9, 2020 17:14
Comment on lines +36 to +41
type sender interface {
exportTrace(ctx context.Context, request *otlptrace.ExportTraceServiceRequest) error
exportMetrics(ctx context.Context, request *otlpmetrics.ExportMetricsServiceRequest) error
exportLogs(ctx context.Context, request *otlplogs.ExportLogsServiceRequest) error
stop() error
}
Copy link
Member

Choose a reason for hiding this comment

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

This interface is no longer required, was added with the idea that both grpc and http will be in the same exporter. Needs to be removed from grpc as well.

Copy link
Member

Choose a reason for hiding this comment

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

Created the PR to remove from grpc #1933

@bogdandrutu bogdandrutu merged commit c95a420 into open-telemetry:master Oct 13, 2020
@tigrannajaryan tigrannajaryan deleted the feature/tigran/otlp_http branch October 14, 2020 13:29
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