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

Fix OpenTelemetry.Tracer.end_span return type spec #796

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RudolfMan
Copy link

Hi there!

Based on the specs of otel_span:end_span/0,1 this Tracer.end_span/0,1 is supposed to always return span context (:undefined is not an option for the return)

See here:

-spec end_span(SpanCtx) -> SpanCtx when
SpanCtx :: opentelemetry:span_ctx().
end_span(SpanCtx=#span_ctx{span_sdk={Module, _}}) when ?is_recording(SpanCtx) ->
_ = Module:end_span(SpanCtx, undefined),
SpanCtx#span_ctx{is_recording=false};
end_span(SpanCtx) ->
SpanCtx.
%% @doc Ends the given span context with the given timestamp.
%%
%% If `SpanCtx' is not recording, this function doesn't do anything.
%% If `Timestamp' is `undefined', this is equivalent to {@link end_span/1}.
%% Returns the updated span context.
-spec end_span(SpanCtx, Timestamp) -> SpanCtx when
SpanCtx :: opentelemetry:span_ctx(),
Timestamp :: integer() | undefined.

@tsloughter
Copy link
Member

I believe the otel_tracer.erl may be the incorrect one as SpanCtx could be undefined. Not positive yet, but that is my guess here.

@tsloughter
Copy link
Member

@RudolfMan any update? I think you actually need to update otel_tracer to have undefined as a valid return of end_span.

@RudolfMan RudolfMan force-pushed the fix_tracer_end_span_typespec branch from 30db7a8 to 080fbc1 Compare November 22, 2024 13:28
@RudolfMan RudolfMan force-pushed the fix_tracer_end_span_typespec branch from 080fbc1 to e832dff Compare November 22, 2024 13:30
@RudolfMan
Copy link
Author

Just updated, @tsloughter!

@RudolfMan RudolfMan force-pushed the fix_tracer_end_span_typespec branch from e832dff to 913e583 Compare November 26, 2024 19:48
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.32%. Comparing base (0a969d1) to head (913e583).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #796   +/-   ##
=======================================
  Coverage   17.32%   17.32%           
=======================================
  Files          24       24           
  Lines         710      710           
=======================================
  Hits          123      123           
  Misses        587      587           
Flag Coverage Δ
api 17.32% <ø> (ø)
elixir 17.32% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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