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

use self() for grpc channel name #481

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

tsloughter
Copy link
Member

using ?MODULE limited us to a single grpcbox client channel. We may want to not have a channel for each Signal (traces, meitrcs, logs) if they are the same configuration, but for now this at least allows each to export -- before only 1 could init their exporter because it would conflict with the other.

Reuse of the channel is not as simple as using ?MODULE and if it is already started use that channel because the endpoint may be different. Not only that, the configuration to the endpoint may be different, so a Channel name would have to be all the distiguishing attributes of a Channel... Just easier to create one for each Signal.

using ?MODULE limited us to a single grpcbox client channel. We
may want to not have a channel for each Signal (traces, meitrcs,
logs) if they are the same configuration, but for now this at
least allows each to export -- before only 1 could init their
exporter because it would conflict with the other.

Reuse of the channel is not as simple as using ?MODULE and if
it is already started use that channel because the endpoint may
be different. Not only that, the configuration to the endpoint
may be different, so a Channel name would have to be all the
distiguishing attributes of a Channel... Just easier to create
one for each Signal.
@tsloughter tsloughter requested a review from a team October 20, 2022 23:48
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 37.20% // Head: 9.32% // Decreases project coverage by -27.87% ⚠️

Coverage data is based on head (2515247) compared to base (f5084c1).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #481       +/-   ##
==========================================
- Coverage   37.20%   9.32%   -27.88%     
==========================================
  Files          57       6       -51     
  Lines        3508    1995     -1513     
==========================================
- Hits         1305     186     -1119     
+ Misses       2203    1809      -394     
Flag Coverage Δ
api ?
elixir ?
erlang 9.32% <83.33%> (-27.85%) ⬇️
exporter 7.64% <83.33%> (+0.04%) ⬆️
sdk ?
zipkin 54.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ntelemetry_exporter/src/opentelemetry_exporter.erl 61.43% <83.33%> (+0.25%) ⬆️
apps/opentelemetry_api/src/otel_span.erl
apps/opentelemetry/src/otel_simple_processor.erl
apps/opentelemetry/src/otel_events.erl
apps/opentelemetry/src/otel_batch_processor.erl
apps/opentelemetry/src/otel_exporter_stdout.erl
...opentelemetry_api/src/otel_propagator_text_map.erl
apps/opentelemetry_api/src/otel_tracer_noop.erl
apps/opentelemetry/src/otel_exporter.erl
apps/opentelemetry/src/otel_tracer_default.erl
... and 42 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

%% Channel name can be any term. To separate Channels per
%% the process calling the exporter (like the batch processor,
%% metric reader and log handler) use the current pid
Channel = self(),
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to use {self(), ?MODULE} to also avoid any sort of problems with multiple sorts of callers? I guess it wouldn't because the channelpid is stored but yeah.

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't hurt but don't think it'd add anything yea

@tsloughter tsloughter merged commit 90f2a5b into open-telemetry:main Oct 21, 2022
@tsloughter tsloughter deleted the grpc-channel-name branch October 21, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants