Method to add service_name to all spans

[isobelhooper]

This commit adds a with_service_name method to PipelineBuilder and Exporter to allow users to set the service name in one place, following the behaviour of the Datadog and Zipkin exporters.

It also sets a default service.name value if the user does not configure one.

---

I noticed that the doc test for with_trace_config already handles this particular case, but having a method specifically for it would match other exporters, and would also make it possible to supply a default value.

[frigus02]

Thanks a lot. I think this makes sense to me. I want to have another look at the spec to remind myself how service.name is supposed to work. Will try to do this tomorrow.

[frigus02]

I looked a bit closer now. My thoughts are:

• Defaulting to "unknown_service" is good. According to the spec we should handle this in the SDK somehow.

• Both the Zipkin and Jaeger exporter should use the service.name from the span attributes. I don't think they do, so they are currently not spec compliant.

For Application Insights we use the service.name to set the Cloud role context tag. As far as I know this tag is optional. But I don't know if there is a best practice around setting this to some default value. If there is, it definitely makes sense to add that here.

Based on this my feeling is that:

• We should add the default value for service.name to opentelemetry-rust.

• We should fix the Zipkin and Jaeger exporter so they take the service.name attribute into account.

• We can add a convenience function to the pipeline builder here to set the service name. I think I would prefer if that would add the service.name to the trace config. But I think this might not work because the resource is behind an Arc.

I'm happy to help with any of those.

What do you think?

[isobelhooper]

Thank you for your detailed reply, and apologies for not replying sooner!

Based on this my feeling is that:

• We should add the default value for service.name to opentelemetry-rust.
• We should fix the Zipkin and Jaeger exporter so they take the service.name attribute into account.
• We can add a convenience function to the pipeline builder here to set the service name. I think I would prefer if that would add the service.name to the trace config. But I think this might not work because the resource is behind an Arc.

I'm happy to help with any of those.

What do you think?

I've talked through this with my coworker who knows more Rust than I do (I'm afraid I'm quite new at it), and he and I can work on rewriting the convenience function for the pipeline builder so that it adds service.name to the trace config.

Based on the specification for resources, it looks like the correct behaviour if someone sets service.name in both with_trace_config and with_service_name is to take the name from the later of the calls, so we'll bear that in mind.

The other two changes (adding the correct behaviour to opentelemetry-rust, and fixing the Zipkin and Jaeger exporters) both sound good, but I'm not sure I have the bandwidth or the knowledge of Rust to help with them.

Would it be okay to implement the spec-compliant default in opentelemetry-application-insights for now, and then move it over to opentelemetry-rust later? I can include that as part of this PR as well.

[frigus02]

Hi @isobelhooper, sorry for the delayed response. This looks great, thank you. I'm going to create a new release with this soon.

Regarding the Zipkin/Jaeger changes: don't worry about it. It's cool if you happen to find the time. I'm sure otherwise someone else is going to do it eventually 🙂

[frigus02]

Just found 2 minor things that we could write differently. I might change that later. But it doesn't block this.

[frigus02]

I just pushed version 0.14.0. Thanks for working on this! 🙂

[frigus02]

Please note that "overriding a previous setting" is not true with the current version 0.13.0 of opentelemetry. We just updated the sdk::Resource::merge logic to reflect the latest spec in open-telemetry/opentelemetry-rust#537. This will be included in the next release, though.