Add decorator to route to provide additional information, including t… by objectiser · Pull Request #1632 · envoyproxy/envoy

objectiser · 2017-09-13T12:41:54Z

…he operation for use with the tracing span.

Related to new Decorator message in envoyproxy/data-plane-api#171

htuch

LGTM

htuch · 2017-09-13T19:48:24Z

The convention in Envoy is DecoratorConstSharedPtr for shared const ptr type name. Maybe add a comment explaining these are created on the main thread at config ingest time and then shared with worked threads as spans come and go.

htuch · 2017-09-13T20:02:26Z

Curious why this changes..

In HttpTracerImpl::startSpan I removed the initialisation of the span name based on the operationName(), as it was not being used in the ZipkinTracer (it was overriding the span name to use the host). I need to check with someone from LightStep to see if it has an impact on their tracer - from initial inspection it seems like the span name is just passed through to the tracer, so I'm guessing Igress/Egress would just appear as the span names in their UI. However this is not that meaningful, as the span.kind can be used to indicate direction.

However this is something that needs to be checked before this PR is merged.

Any update on this? PR looks LGTM otherwise.

I've been in contact with a couple of people at LightStep who said they would try to look at it, but not sure when.

cc @bhs @tedsuo @rnburn Any thoughts appreciated - would ideally like to get this PR merged, so just need to understand if a change in the operation name provided to the LightStep tracer would have any impact on your users?

Per other comment and per @RomanDzhabarov yes it effects the UI. I'm not sure why we need to change the default behavior. If no operation name is specified, do what we do today. If operation name is specified, override it. If tracer wants to override that, no problem.

Also nit: Times(1) is redundant.

RomanDzhabarov · 2017-09-13T20:48:20Z

remove const

RomanDzhabarov · 2017-09-13T23:14:53Z

decorator seems to be pretty generic name, should it be tracing_decorator. And also we need doc update on this.

Using the more general decorator term was @mattklein123 's idea: envoyproxy/data-plane-api#171 (comment)

Will sort out docs soon.

fair enough

RomanDzhabarov · 2017-09-13T23:16:19Z

RomanDzhabarov · 2017-09-13T23:17:05Z

why is this included?

Its required for the apply(Tracing::Span&) on Decorator.

RomanDzhabarov · 2017-09-13T23:19:54Z

what is the span name we are going to report for the ingress?

For zipkin tracer, this was not set - so only change is that the Ingress value would not be provided to LightStep's tracer - so before this PR is merged this needs to be checked with them.

@bhs Could you provide some guidance here?

Prior to this PR, this code was building a span_name based on the 'OperationName` (misleading) which essentially just represents the direction of the request (Ingress or Egress), and if Egress, then it appends the host. However the Zipkin tracer code was ignoring this value, and instead using the host for Egress requests and empty span name for Ingress.

It appears that the span_name is just being passed through to the LightStep tracer and not doing any kind of interpretation of the value to determine span kind. So assume this value is just being included in the LightStep spans and displayed via your UI?

If so, do you have any objections to the changed values (i.e. blank on Ingress and host for Egress), or would you prefer different default values? With this PR, if the configuration includes a specified operation for the route, then this would be used instread.

LS does not care about operation name, but if affects Saved Searches if there are any. And yes that's going to affect UI. Do we really need to change this behavior?
It seems like if there is a decorator on a route, span name will be overwritten and if not, that this default one is going to be used.

That is the problem - the default behaviour is different between the two tracers. LS uses the default value passed in, but the Zipkin tracer overrides the supplied value and just uses the Host for Egress routes.

So not sure how to allow both tracers to maintain their current default behaviour, while at the same time enabling the Decorator to override the span name in both.

Seems better to have one default behaviour across both (all) tracers which is then overridden by the Decorator if defined.

Got it, if there are no common names for spans for LS and Zipkin spans, I'd push the logic of assigning specific span names to the specific tracer (ls and zipkin tracers).

IMO the LS current behavior makes more sense. Meaning, if no operation is set, base operation name off of egress/ingress + host, etc. If the tracer wants to override (as you do inside Zipkin) that seems fine? It doesn't make much sense to me to push this logic entirely into each tracer, but I suppose we could do that. Maybe I'm not fully following the issue here.

@mattklein123 @RomanDzhabarov You are correct - I was still thinking about the previous way I had implemented it, where the tracing config provided operation was passed in when the span was started - whereas now it is set after the span has been created within the Decorator - so all good, I can return the default behaviour to previous versions.

htuch · 2017-09-14T16:38:27Z

@objectiser I'm going to fix it so we consume your envoy-api change now to unblock this PR (and help with one I'm putting together), so should be done Real Soon.

This brings in a bunch of ADS changes, route decorators and the response nonce. This will also unblock envoyproxy#1632.

This brings in a bunch of ADS changes, route decorators and the response nonce. This will also unblock #1632.

This brings in a bunch of ADS changes, route decorators and the response nonce. This will also unblock envoyproxy#1632.

objectiser · 2017-09-15T07:49:06Z

@htuch @mattklein123 rebased off envoy master to make use of the latest data-plane-api.

mattklein123 · 2017-09-15T18:01:52Z

You should be able to store the decorator in a unique_ptr.

mattklein123 · 2017-09-15T18:04:34Z

IMO the LS current behavior makes more sense. Meaning, if no operation is set, base operation name off of egress/ingress + host, etc. If the tracer wants to override (as you do inside Zipkin) that seems fine? It doesn't make much sense to me to push this logic entirely into each tracer, but I suppose we could do that. Maybe I'm not fully following the issue here.

mattklein123 · 2017-09-15T18:06:00Z

Per other comment and per @RomanDzhabarov yes it effects the UI. I'm not sure why we need to change the default behavior. If no operation name is specified, do what we do today. If operation name is specified, override it. If tracer wants to override that, no problem.

Also nit: Times(1) is redundant.

RomanDzhabarov

LGTM

RomanDzhabarov · 2017-09-15T20:40:10Z

nit: will be used as a span name?

RomanDzhabarov · 2017-09-15T20:40:20Z

fair enough

objectiser · 2017-09-16T15:38:59Z

@htuch @mattklein123 As the CLA approach has been changed to use the git commit sign off approach, should I squash the commits and just sign the single commit?

mattklein123 · 2017-09-16T15:41:56Z

@objectiser yeah sorry for trouble just squash/rebase/push with DCO.

…he operation for use with the tracing span. Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2017-09-16T16:28:48Z

@mattklein123 No problem, we're doing the same change in Jaeger.

mattklein123

LGTM just needs some test coverage.

mattklein123 · 2017-09-18T01:27:35Z

+DecoratorConstPtr RouteEntryImplBase::parseDecorator(const envoy::api::v2::Route& route) {
+  DecoratorConstPtr ret;
+  if (route.has_decorator()) {
+    ret = std::unique_ptr<Decorator>(new DecoratorImpl(route.decorator()));


nit: DecoratorConstPtr(...

mattklein123 · 2017-09-18T01:28:55Z

    if (tracing_decision.is_tracing) {
      active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_);
+      if (cached_route_.value() && cached_route_.value()->decorator()) {
+        cached_route_.value()->decorator()->apply(*active_span_);


I think you will need a conn_manager_impl_test to get test coverage for this line.

mattklein123 · 2017-09-18T01:30:10Z

 }

+void LightStepSpan::setOperation(const std::string& operation) {
+  span_.SetOperationName(operation);


Does this need coverage in lightstep tracer?

Not sure best way to test this - the current lightstep tests don't check the operation names passed in via the startSpan method, and there are no accessors for the operation name. Any suggestions welcome.

If it's not easy to verify it's fine just skip it. I would at least make sure it's called just to have basic crash avoidance testing. (It might already be called not sure).

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123

@RomanDzhabarov

mattklein123 · 2017-09-18T18:22:25Z

@RomanDzhabarov any final comments?

RomanDzhabarov

great!

Description: Adds a wrapper script that calls Envoy's comp db generation script suitable for envoy-mobile for use with VS Code. This will create a compile_commands.json file in the root of the repo, allowing for the clangd plugin to pick this up and provide intelisense for native code. Signed-off-by: Snow Pettersen <snowp@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** Removes unused files and unskips a couple tests in the AIGW CLI e2e that were skipped. **Related Issues/PRs (if applicable)** N/A **Special notes for reviewers (if applicable)** N/A --------- Signed-off-by: Ignasi Barrera <nacx@apache.org>

htuch previously approved these changes Sep 13, 2017

View reviewed changes

RomanDzhabarov reviewed Sep 13, 2017

View reviewed changes

objectiser dismissed htuch’s stale review via 4b4b637 September 14, 2017 09:49

htuch added a commit to htuch/envoy that referenced this pull request Sep 14, 2017

config: bump to envoy-api 9be6aff.

77ecc0d

This brings in a bunch of ADS changes, route decorators and the response nonce. This will also unblock envoyproxy#1632.

htuch mentioned this pull request Sep 14, 2017

config: bump to envoy-api 9be6aff. #1645

Merged

mattklein123 pushed a commit that referenced this pull request Sep 14, 2017

config: bump to envoy-api 9be6aff. (#1645)

dec1763

This brings in a bunch of ADS changes, route decorators and the response nonce. This will also unblock #1632.

htuch added a commit to htuch/envoy that referenced this pull request Sep 14, 2017

config: bump to envoy-api 9be6aff.

ea642fc

This brings in a bunch of ADS changes, route decorators and the response nonce. This will also unblock envoyproxy#1632.

objectiser force-pushed the tracing branch from adaec44 to de087c3 Compare September 14, 2017 22:27

mattklein123 reviewed Sep 15, 2017

View reviewed changes

objectiser force-pushed the tracing branch from de087c3 to ff19c23 Compare September 15, 2017 20:22

RomanDzhabarov reviewed Sep 15, 2017

View reviewed changes

Add decorator to route to provide additional information, including t…

6847567

…he operation for use with the tracing span. Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the tracing branch from 7f9a5df to 6847567 Compare September 16, 2017 16:26

mattklein123 reviewed Sep 18, 2017

View reviewed changes

objectiser added 2 commits September 18, 2017 12:22

Use DecoratorConstPtr and add test for decorator being called.

19c446b

Signed-off-by: Gary Brown <gary@brownuk.com>

Add call to span->setOperation in lightstep test

43530f4

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 approved these changes Sep 18, 2017

View reviewed changes

RomanDzhabarov approved these changes Sep 18, 2017

View reviewed changes

RomanDzhabarov merged commit 89982e3 into envoyproxy:master Sep 18, 2017

objectiser deleted the tracing branch September 19, 2017 07:33

objectiser mentioned this pull request Sep 19, 2017

tracing: RFC Allowing span operation name to be determined based on HTTP path #1527

Closed

Conversation

objectiser commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

objectiser Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Sep 14, 2017

Uh oh!

objectiser commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

objectiser commented Sep 16, 2017

Uh oh!

mattklein123 commented Sep 16, 2017

Uh oh!

objectiser commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

objectiser commented Sep 13, 2017 •

edited

Loading

RomanDzhabarov Sep 14, 2017 •

edited

Loading

objectiser Sep 14, 2017 •

edited

Loading

objectiser commented Sep 16, 2017 •

edited

Loading