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

Exporters shouldn't have service name parameter #1607

Closed
srikanthccv opened this issue Feb 13, 2021 · 9 comments
Closed

Exporters shouldn't have service name parameter #1607

srikanthccv opened this issue Feb 13, 2021 · 9 comments
Assignees
Labels
1.0.0rc2 release candidate 2 for tracing GA bug Something isn't working exporters good first issue Good first issue help wanted

Comments

@srikanthccv
Copy link
Member

Exporters jaeger, zipkin and opencensus accept an argument service_name but ideally they should rely on the Resource's service.name attribute from Tracer/Span to indicate the resource that produced the corresponding telemetry.

@srikanthccv srikanthccv added the bug Something isn't working label Feb 13, 2021
@lzchen lzchen added the 1.0.0rc2 release candidate 2 for tracing GA label Feb 18, 2021
@dmarar
Copy link
Contributor

dmarar commented Feb 22, 2021

@lonewolf3739 , pls assign this to me

@srikanthccv
Copy link
Member Author

@dmarar I don't have permissions to assign. @lzchen / @codeboten can assign this to you.

@codeboten
Copy link
Contributor

@dmarar assigned! thanks

@dmarar
Copy link
Contributor

dmarar commented Feb 22, 2021

@lonewolf3739 , Just to clarify on the requirement:
consider the following export() function of jaeger:

def export(self, spans) -> SpanExportResult:
>       service_name = spans[0].resource.service.name if spans[0].resource  else None  #assuming that all spans in a batch will have same resource.service.name

        translator = Translate(spans)
        if self.transport_format == TRANSPORT_FORMAT_PROTOBUF:
>            pb_translator = ProtobufTranslator(**service_name**)
            jaeger_spans = translator._translate(pb_translator)
            batch = model_pb2.Batch(spans=jaeger_spans)
            request = PostSpansRequest(batch=batch)
....

@srikanthccv
Copy link
Member Author

@dmarar
You can just get the resource from trace.get_tracer_provider() and populate service name and resource attributes.

it would look something like this

from opentelemetry import trace
from opentelemetry.sdk.resources import SERVICE_NAME

tracer_provider = trace.get_tracer_provider()
resource = tracer_provider.resource
svc_name = resource.attributes[SERVICE_NAME]

@lzchen
Copy link
Contributor

lzchen commented Feb 24, 2021

Should we remove service.name from the constructors of exporters too?

@srikanthccv
Copy link
Member Author

Should we remove service.name from the constructors of exporters too?

Yes, that's what I mentioned in the issue title, I hope that's conveying the same? We need to remove the service_name param and use the Resource attribute.

@dmarar dmarar mentioned this issue Mar 3, 2021
11 tasks
@dmarar
Copy link
Contributor

dmarar commented Mar 3, 2021

@lonewolf3739 - I have created a draft PR 1669
As said i have made the changes to OpenCensus and Jaeger.
But from what i see , zipkin the changes are already in place.

I did not find any other place where the service_name is being used in exporters.
Could you please point out if any other places i have missed?

Thanks,
-Dilip M

@codeboten
Copy link
Contributor

This was fixed by #1669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0rc2 release candidate 2 for tracing GA bug Something isn't working exporters good first issue Good first issue help wanted
Projects
None yet
Development

No branches or pull requests

4 participants