Set zipkin_service_name to LocalInfo::clusterName(). by g-easy · Pull Request #7354 · envoyproxy/envoy

g-easy · 2019-06-21T07:04:27Z

No description provided.

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

objectiser

LGTM - might be worth adding a test for a supplied clusterName appearing in the span?

mattklein123

Nice, generally LGTM but +1 for adding a test.

/wait

api/envoy/config/trace/v2/trace.proto

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

g-easy · 2019-06-24T04:40:34Z

LGTM - might be worth adding a test for a supplied clusterName appearing in the span?

Nothing like this is exposed for testing by OpenCensus, which has a very narrow API by design.

The cluster name is fed to the Zipkin exporter, which puts it into the JSON it generates. The cluster name doesn't appear in the generated SpanData which we actually do have access to in tests.

I'm not really sure how to meaningfully test this. I could add a getClusterName to OpenCensus::Driver. What do you suggest?

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

objectiser · 2019-06-24T08:37:26Z

@g-easy One possibility would be to have a test create the tracer with a mock exporter, and then create a test span to check the exported data?

stale · 2019-07-01T08:52:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-07-08T09:14:09Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

g-easy · 2019-07-09T04:04:16Z

I'm sorry @objectiser I don't follow - could you show me what this would look like in the code?

Alternatively, we could submit this as-is, it's just swapping out a string.

objectiser · 2019-07-09T07:40:40Z

@g-easy didn't have a concrete idea - although thought if possible to mock the exporter could then capture the span data reported from the tracer - but on examining the code it doesn't look like this would work.

So I'd be ok with merging the change for now.

@mattklein123 ok with you?

g-easy · 2019-07-09T07:45:00Z

Yeah, sorry, the OpenCensus C++ API is very limited by design. :/

objectiser · 2019-07-09T07:55:53Z

Looks like some form of "in memory" exporter has been requested in other languages census-instrumentation/opencensus-java#1640 census-instrumentation/opencensus-go#293 - if this was available it would have solved the problem :)

g-easy · 2019-07-09T08:25:35Z

We sort of do that here to catch the generated SpanData:

envoy/test/extensions/tracers/opencensus/tracer_test.cc

Line 55 in 1faaed8

// use it to catch the spans we produce.

The problem is the cluster name doesn't end up in the SpanData, it only ends up in the Zipkin-specific JSON generated by the Zipkin-specific exporter. I tried to say this in #7354 (comment)

mattklein123 · 2019-07-09T14:03:16Z

@mattklein123 ok with you?

Sure that's fine.

mattklein123 · 2019-07-09T14:03:33Z

@g-easy please merge master.

/wait

…vice Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

g-easy · 2019-07-11T04:48:23Z

source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

  if (oc_config.zipkin_exporter_enabled()) {
    ::opencensus::exporters::trace::ZipkinExporterOptions opts(oc_config.zipkin_url());
-    opts.service_name = oc_config.zipkin_service_name();
+    opts.service_name = local_info_.clusterName();


Before we merge this: what actually populates clusterName?

I have a test config with:

static_resources: clusters: - name: cluster1

And I get empty string instead of "cluster1" on L206.

It's populated with the local cluster name. This can be set via CLI or in the bootstrap config. https://www.envoyproxy.io/docs/envoy/latest/operations/cli#cmdoption-service-cluster

LMK if this change still looks good to you.

/wait-any

Thanks for the tip, setting the name with --service-cluster totally works!

I'm happy with this change now.

g-easy · 2019-07-17T02:13:28Z

Please go ahead and merge. :)

RashmiRam · 2020-09-18T10:05:57Z

Since this configurable option of service name is removed, Is there any other option to configure the zipkin service name ?
@g-easy Can you please help here?

g-easy · 2020-09-22T04:37:11Z

Are you able to set the service name you need with --service-cluster?

RashmiRam · 2020-09-22T04:44:33Z

I haven't tried it. But, I don't want to change the cluster name since that is referenced in some other systems. I want to change the service name in tracing alone. Would that be possible with this param?

g-easy · 2020-09-22T05:17:10Z

Unfortunately, there's currently no override for the service name.

RashmiRam · 2020-09-22T05:28:25Z

Do you think it is a valid ask to bring back this service name overriding in tracing feature?

g-easy · 2020-09-22T05:46:31Z

It's worth asking the Envoy folks. Maybe open an issue?

g-easy mentioned this pull request Jun 21, 2019

Add initial OpenCensus tracing support. #5387

Merged

Set zipkin_service_name to LocalInfo::clusterName().

2704299

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

g-easy force-pushed the service branch from cb34d9f to 2704299 Compare June 21, 2019 07:36

objectiser previously approved these changes Jun 21, 2019

View reviewed changes

mattklein123 requested changes Jun 21, 2019

View reviewed changes

api/envoy/config/trace/v2/trace.proto Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 21, 2019

mattklein123 self-assigned this Jun 21, 2019

Reserve tag 7.

fc03502

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

g-easy dismissed objectiser’s stale review via fc03502 June 24, 2019 03:39

repokitteh-read-only bot removed the waiting label Jun 24, 2019

format

35623b3

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

mattklein123 added the waiting label Jun 24, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 1, 2019

stale bot closed this Jul 8, 2019

mattklein123 reopened this Jul 9, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 9, 2019

Merge branch 'master' of https://github.com/envoyproxy/envoy into ser…

65bfddc

…vice Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>

repokitteh-read-only bot removed the waiting label Jul 11, 2019

g-easy commented Jul 11, 2019

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Jul 11, 2019

mattklein123 approved these changes Jul 17, 2019

View reviewed changes

mattklein123 merged commit ea3ebca into envoyproxy:master Jul 17, 2019

g-easy deleted the service branch July 17, 2019 05:12

g-easy mentioned this pull request Oct 2, 2019

Upgrade io_opencensus_cpp. #8340

Merged

Conversation

g-easy commented Jun 21, 2019

Uh oh!

objectiser left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

g-easy commented Jun 24, 2019

Uh oh!

objectiser commented Jun 24, 2019

Uh oh!

stale bot commented Jul 1, 2019

Uh oh!

stale bot commented Jul 8, 2019

Uh oh!

g-easy commented Jul 9, 2019

Uh oh!

objectiser commented Jul 9, 2019

Uh oh!

g-easy commented Jul 9, 2019

Uh oh!

objectiser commented Jul 9, 2019

Uh oh!

g-easy commented Jul 9, 2019

Uh oh!

mattklein123 commented Jul 9, 2019

Uh oh!

mattklein123 commented Jul 9, 2019

Uh oh!

g-easy Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

g-easy Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

g-easy commented Jul 17, 2019

Uh oh!

RashmiRam commented Sep 18, 2020

Uh oh!

g-easy commented Sep 22, 2020

Uh oh!

RashmiRam commented Sep 22, 2020

Uh oh!

g-easy commented Sep 22, 2020

Uh oh!

RashmiRam commented Sep 22, 2020

Uh oh!

g-easy commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants