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

Span Processors are basically undocumented #397

Closed
Tracked by #775
genebean opened this issue Sep 18, 2020 · 5 comments · Fixed by #461
Closed
Tracked by #775

Span Processors are basically undocumented #397

genebean opened this issue Sep 18, 2020 · 5 comments · Fixed by #461
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed stale

Comments

@genebean
Copy link
Contributor

I have learned via chat that the SimpleSpanProcessor is not intended for anything other than local testing yet its all that is used in the examples. By trial and error I learned that the expected default is the BatchSpanProcessor. Having said that, I am not aware of any documentation of this or of when or how to use any of the span processors.

For me, this became a real issue today: the SimpleSpanProcessor paired with the Jaeger::CollectorExporter broke a service for us - it made an endpoint response time go from sub-1s to 20s. The batch processor fixed our issue. I also think there were additional issues that are harder to quantify too, fwiw.

@fbogsany fbogsany added this to the Beta v0.7 milestone Sep 18, 2020
@fbogsany fbogsany added help wanted Extra attention is needed good first issue Good for newcomers labels Sep 21, 2020
@rachelleahklein
Copy link

I'm planning to help provide some documentation on this topic.

@genebean
Copy link
Contributor Author

FWIW, I still feel that notable amounts of documentation are missing here. #461 only made changes in the comments of two of the four span processors. Additionally, two are located in https://github.com/open-telemetry/opentelemetry-ruby/tree/master/sdk/lib/opentelemetry/sdk/trace while the other two are located in https://github.com/open-telemetry/opentelemetry-ruby/tree/master/sdk/lib/opentelemetry/sdk/trace/export yet neither the trace nor the export folder contain a readme or other centralized documentation on the processors. While the additions are helpful and welcome, they do not provide what I think is needed for the average user of this project, myself included. I feel like there needs to be a readme somewhere, even if all it does is reference something over in the spec repo.

@fbogsany fbogsany reopened this Nov 16, 2020
@fbogsany fbogsany removed this from the Beta v0.9 milestone Jan 11, 2021
@fbogsany fbogsany added this to the Tracing v1.0 milestone Feb 18, 2021
@fbogsany fbogsany removed this from the Tracing v1.0 milestone Jul 13, 2021
@plantfansam
Copy link
Contributor

Is this still an active issue? We don't have a pattern of READMEs in sdk/ subdirectories, so I'd be hesitant to add one just for this. Perhaps we could log a silenceable warning in add_span_processor if you register a SimpleSpanProcessor. Something like

        # Adds a new SpanProcessor to this {Tracer}.
        #
        # @param span_processor the new SpanProcessor to be added.
        def add_span_processor(span_processor)
          @mutex.synchronize do
            if span_processor.is_a?(SimpleSpanProcessor) && !ENV["OTEL_RUBY_SILENCE_SPAN_PROCESSOR_WARNING"].nil?
              OpenTelemetry.logger.warn('you are using SimpleSpanProcessor are you sure about that, pal?')
            end
            if @stopped
              OpenTelemetry.logger.warn('calling Tracer#add_span_processor after shutdown.')
              return
            end
            @span_processors = @span_processors.dup.push(span_processor)
          end
        end

@ahayworth
Copy link
Contributor

@plantfansam Yes, I do think this is still relevant actually. Outside of the default, happy-path case, it's hard to get started in my opinion.

Of course, many language SIGs have issues with documentation, but I think we could keep this open as a docs TODO.

Copy link
Contributor

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.

@github-actions github-actions bot added the stale label Apr 27, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants