Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Add tag for service #119

Merged
merged 5 commits into from
Jul 12, 2018
Merged

Add tag for service #119

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7d3740
Add tag for service
tylerbenson May 30, 2018
1b0aef8
Try to incorporate feedback.
tylerbenson Jun 25, 2018
c18157d
Removed “being called by a client”
tylerbenson Jul 5, 2018
307542f
limit scope of service cascading
tylerbenson Jul 6, 2018
1b3c89e
Adopt description changes suggested by @tedsuo.
tylerbenson Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# History

## 1.2 (2018/05/30)

- Added service tag

## 1.1 (2017/03/19)

- Added message bus tags
Expand Down
3 changes: 2 additions & 1 deletion semantic_conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ Span tags apply to **the entire Span**; as such, they apply to the entire timera
| `peer.ipv4` | string | Remote IPv4 address as a `.`-separated tuple. E.g., `"127.0.0.1"` |
| `peer.ipv6` | string | Remote IPv6 address as a string of colon-separated 4-char hex tuples. E.g., `"2001:0db8:85a3:0000:0000:8a2e:0370:7334"` |
| `peer.port` | integer | Remote port. E.g., `80` |
| `peer.service` | string | Remote service name (for some unspecified definition of `"service"`). E.g., `"elasticsearch"`, `"a_custom_microservice"`, `"memcache"` |
| `peer.service` | string | Remote service name (for some unspecified definition of `"service"`). E.g., `"elasticsearch"`, `"a_custom_microservice"`, `"memcache"`. Should correspond with values set in `service`. |
| `sampling.priority` | integer | If greater than 0, a hint to the Tracer to do its best to capture the trace. If 0, a hint to the trace to not-capture the trace. If absent, the Tracer should use its default sampling mechanism. |
| `service` | string | The name of the current service. E.g., `"elasticsearch"`, `"a_custom_microservice"`, `"memcache"`. Should correspond with values set in `peer.service`. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have a description of the use case, such as a service mesh creating spans on behalf of different services.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a single use-case that would work well in this description. For example, with Datadog, (in addition to the use case you described) someone could set the service name via this tag that would override the default service name for a specific tag with the goal of separating out or correcting the name. Do you have a better description in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your use case is less clear to me. Why would a service override its name? In our internal wrappers around oss jaeger libs we do the opposite, ie prevent services from even configuring their name in the tracer, instead we read it from env vars set by infra, to avoid names that do not correspond to discovery names.

I think it would be good to go through an RFC for this feature and describe the use cases it is meant for and why it's needed. Otherwise people will just unintentionally abuse it, I'm afraid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are two use cases for wanting the ability to override from the globally configured name:

First: you can think of like the service mesh use case (creating a span on behalf of another service). For example when making a database call, we rename that span to correspond to the database instead of the current service.

Second: same issue applies when dealing with a java application server with multiple war files deployed onto it. Many people think of those war files as logically separate things and want them to be named differently. If the ENV configured name takes priority, there isn't a nice way to make that distinction.

What about the name and use case is unclear? Why the need for an RFC? @tedsuo thoughts?

I understand your suggestion for configuring separate tracer instances to cover some of these purposes, but that doesn't work well in auto-instrumentation land where things need to be much more dynamic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for RFC is that it serves as a formal description of the use cases that can be considered canonical. The summary table modified in this PR can refer to the RFC for more details. Otherwise defining this new tag without explaining how to use it leads to confusion of which I think your database use case is a perfect example: it's misleading to attribute the client-side span inside the application to the database server, and unnecessary since the database can and should be referenced in the peer.service tag instead, per already existing semantic guidelines.

The multi-war file example could be a use case, but none of the existing instrumentation in opentracing/contrib behaves like that today, i.e. if I use java-grpc or java-spring-cloud or probably any other java-xxx modules from contrib, they won't know how to set the service tag because they expect the tracer to do it. So for that multi-war example to be practical not only do we need to introduce the service tag, but we also need to change all instrumentations in contrib to be able to take service name as an argument so that they can set this tag. I think that's much larger implications than just adding an optional tag. Personally I don't think we should be recommending this as a valid use case, since it can be much easier solved with tracer per application, even if they are deployed in a single app server (any decent app server would offer a separate class loader for each app).

So that leaves us with service mesh or similar use cases where an application is constructing spans on behalf of other applications, which is indeed a marginal use case well served by an "optional" tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely want this in order for service meshes and collection systems to trace "on behalf" of another process. There is a need to differentiate between their own tracing and the tracing they are doing on behalf of another service. I would recommend that be listed as a primary use case.

I don't think you would ever have RPC instrumentation setting this. In the case of running multiple processes/JARs/etc, it would be an application-level span that would receive this tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest focusing just on the service mesh /external reporting. The guidance should be, IMHO:

  • when describing a span which is associated with an adjacent service the current service is calling, use the peer.service tag.
  • when recording a span out-of-process, on behalf of another service, use the service tag.

Separately, there is a larger issue around tracing stateful "things", be they called services, processes, or objects. Right now tracing is only about transactions. We consistently see code that persists and interacts with multiple traces/transactions, and it is not possible to model them effectively right now. For that issue, I think we need an RFC. And WARs, to me, would fall under that scenario.

I would recommend we separate that larger case out from the smaller (yet also necessary) case of "external tracing" such as service meshes, syslog-to-trace, etc. The service tag is sufficient for that narrower use case.

Copy link
Contributor Author

@tylerbenson tylerbenson Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro @tedsuo if you want to give me the text you want in the description column that you'd like to see, I can put it in there. I'm not clear on how to word the description so it would be useful for the service mesh use case. The description ted gave above seems too long for that compared to the other descriptions already there.

| `span.kind` | string | Either `"client"` or `"server"` for the appropriate roles in an RPC, and `"producer"` or `"consumer"` for the appropriate roles in a messaging scenario. |

### Log fields table
Expand Down
1 change: 1 addition & 0 deletions semantic_conventions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ standard_tags:
'peer.port': integer
'peer.service': string
'sampling.priority': integer
'service': string
'span.kind': string

standard_log_fields:
Expand Down