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

Message Oriented Middleware (MOM) tags #49

Merged
merged 4 commits into from
Mar 8, 2017
Merged

Conversation

objectiser
Copy link
Contributor

No description provided.


The following Span tags combine to model Message Oriented Middleware communications:

- `mom.destination` and `mom.type`: as described in the table above
Copy link
Member

Choose a reason for hiding this comment

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

Can you give more details about how to set mom.destination? e.g. when the client sends a message for kafka.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the text in the main table, as this is where most of the examples seem to be. Let me know if the text is clear enough.

Copy link
Member

Choose a reason for hiding this comment

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

Better, now.

@wu-sheng
Copy link
Member

wu-sheng commented Mar 2, 2017

Still concern about MOM, no better suggestion yet.

@objectiser
Copy link
Contributor Author

It is a common term and abbreviation: https://en.wikipedia.org/wiki/Message-oriented_middleware

However if a better abbreviation can be found I'm happy to change.

@wu-sheng
Copy link
Member

wu-sheng commented Mar 2, 2017

@objectiser the MOM is fine. Checked on Oracle Java System Message Queue page site: https://docs.oracle.com/cd/E19575-01/820-6424/6nhokb1hi/index.html

@yurishkuro
Copy link
Member

I don't have strong objections to the new tags per se, but we haven't talked much about how OT can be used in the messaging scenarios, and don't have any documentation or examples about how these use cases can be modeled, so standardizing the tags feels a bit premature.

@objectiser if you have already implemented MOM scenarios, maybe you can share the experience (e.g. in a separate issue)? It could make for a good section on http://opentracing.io/documentation/pages/instrumentation/common-use-cases.html. I would like to see some prescription of the flow that tracers should support, as otherwise introducing "standard" tags is a moot point.

@objectiser
Copy link
Contributor Author

@yurishkuro Sounds reasonable - I'll have a go at creating a section in the use case section.

@@ -25,13 +25,15 @@ Span tags apply to **the entire Span**; as such, they apply to the entire timera
| `http.method` | string | HTTP method of the request for the associated Span. E.g., `"GET"`, `"POST"` |
| `http.status_code` | integer | HTTP response status code for the associated Span. E.g., 200, 503, 404 |
| `http.url` | string | URL of the request being handled in this segment of the trace, in standard URI format. E.g., `"https://domain.net/path/to?resource=here"` |
| `mom.destination` | string | An address at which messages can be exchanged. E.g. A Kafka record has an associated `"topic name"` that can be extracted by the instrumented producer or consumer and stored using this tag. |
Copy link
Contributor

Choose a reason for hiding this comment

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

a nit, but I worry that 'mom.' is not going to be sufficiently-widely-recognized as an acronym... mq. is tempting, though maybe it's not general enough.

Copy link

@devinsba devinsba Mar 2, 2017

Choose a reason for hiding this comment

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

To Ben's point, if I hadn't read the name of the pull request I wouldn't know what the acronym meant, maybe mb for message bus, or ms for message system, though the second could be confusing for windows systems

Choose a reason for hiding this comment

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

How about just messaging?

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 think any acronym used in this area would be unfamiliar - e.g. mb would still need to be explained. So we either provide clear description (and link if necessary), or don't use acronym as @jpkrohling suggests. But then should database be used instead of db - I know it is more commonly understood, but just thinking of consistency of naming style.


- `mom.destination` and `mom.type`: as described in the table above
- `span.kind`: either `"producer"` or `"consumer"`. It is important to provide this tag **at Span start time**, as it may affect internal ID generation.
- `error`: whether the message exchange ended in an error
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this could be left out... it seems (?) clear to me that error can apply basically anywhere.

The following Span tags combine to model Message Oriented Middleware communications:

- `mom.destination` and `mom.type`: as described in the table above
- `span.kind`: either `"producer"` or `"consumer"`. It is important to provide this tag **at Span start time**, as it may affect internal ID generation.
Copy link
Contributor

Choose a reason for hiding this comment

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

(nice/interesting to see span.kind take on some new possible values)

@@ -55,6 +57,15 @@ The following Span tags combine to model RPCs:
- `error`: whether the RPC ended in an error
- `peer.hostname`, `peer.ipv4`, `peer.ipv6`, `peer.port`, `peer.service`: optional tags that describe the RPC peer (often in ways it cannot assess internally)

### Message Oriented Middleware

The following Span tags combine to model Message Oriented Middleware communications:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be worth throwing in a sentence about the FOLLOWS_FROM reference type, since that typically what people will want to use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree - and was planning on discussing that in the use case section, which I will hopefully be able to write tomorrow.

@@ -25,13 +25,15 @@ Span tags apply to **the entire Span**; as such, they apply to the entire timera
| `http.method` | string | HTTP method of the request for the associated Span. E.g., `"GET"`, `"POST"` |
| `http.status_code` | integer | HTTP response status code for the associated Span. E.g., 200, 503, 404 |
| `http.url` | string | URL of the request being handled in this segment of the trace, in standard URI format. E.g., `"https://domain.net/path/to?resource=here"` |
| `mom.destination` | string | An address at which messages can be exchanged. E.g. A Kafka record has an associated `"topic name"` that can be extracted by the instrumented producer or consumer and stored using this tag. |
| `mom.type` | string | Message destination type, e.g. `"topic"` or `"queue"`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

db.type is cassandra/redis/etc...

  1. I like the idea of allowing for that sort of mom.type here, too
  2. maybe this could be mom.dest_class and mom.destination could be mom.dest_name???

Copy link
Contributor

Choose a reason for hiding this comment

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

(not sure about my actual suggestions, though I do think that mom.type and db.type should be the same sort of type :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand your point - I did wonder about the consistent use of type - but also not sure of the relationship of db.type with component. e.g. db.type could be sql and nosql with component tag being cassandra/redis/etc.

Copy link
Member

Choose a reason for hiding this comment

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

I did not like the db.type=sql/nosql , db.type related of db.statement. If tracer want to analysis statement, db.type=nosql is not helpful for anything.

By using component tag, I prefer the value is what framework component the application used. e.g. for redis in Java, component = Jedis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok good point :)
So possibly mom.type is appropriate, as db.type enables better understanding of the information from db.statement - similarly mom.type enables a better understanding of the messaging pattern being used.

So as a general rule, any tag xxx.type is to aid higher level interpretation of the span or set of related spans, on which that type is defined?

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I prefer this.

BTW. Only speak for my implementation(not related to the Spec or your PR), I add a new tag, named layer, to help analysis program known which set of tag keys are possible existed.

@wu-sheng
Copy link
Member

wu-sheng commented Mar 3, 2017

By so many acronyms or abbreviations existed in tags or log fields, I want to propose about adding an KEY CODE(integer value) for each key. e.g. db.instance keyCode=10; db.statement keyCode=11. Like other spec code, we can retain some code(0-10000) for OT official keys.

After we have these, all platform implementations, can use the integer to judge. In my tracer implementations, we have an analysis backend, analysis the service-flow by some tags, right now, it must be based on String.equal() and consider char-case... I hope we can improve on that.

@bhs @objectiser thoughts?

#50

@yurishkuro
Copy link
Member

@wu-sheng the keyCode proposal is not directly related to this PR, I suggest opening a different issue.

@wu-sheng
Copy link
Member

wu-sheng commented Mar 3, 2017

@yurishkuro I will start a new issue on that.

@objectiser
Copy link
Contributor Author

Added messaging usecase: opentracing/opentracing.io#189

After working on this, I think it would be better to drop the mom.type for now, as not sure it is necessarily required. Can always be added later if there is a need.

Also as only providing the destination, it looked more natural to use message.destination - so will update this PR with these changes.

Copy link
Contributor

@bhs bhs left a comment

Choose a reason for hiding this comment

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

apologies about turning the nitpicking up to 11 here... I want to be really careful about this file.

- `peer.hostname`, `peer.ipv4`, `peer.ipv6`, `peer.port`, `peer.service`: optional tags that describe the RPC peer (often in ways it cannot assess internally)

### Messaging
Copy link
Contributor

@bhs bhs Mar 4, 2017

Choose a reason for hiding this comment

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

I am really sorry to be so pedantic, but changes in this file require that :-/

"Messaging" just seems too general... e.g., it would include RPC as a (simple) form of message-passing. I realize that I also complained that "MOM" was too obscure/overloaded.

I would vote for "PubSub" here and "pubsub." as a prefix. I realize that's not a perfect fit (doesn't express MQ), either, but seems like it get the point across?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you say, it doesn't express mq, which I think would cause confusion. So how about a compromise, "Message Bus", as suggested by @devinsba, but messagebus (or message_bus.) prefix, as mb is not a well known abbreviation?

Copy link
Contributor

@bhs bhs Mar 4, 2017

Choose a reason for hiding this comment

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

either message_bus.destination or message_bus.dest SGTM

@@ -8,25 +8,27 @@

standard_tags:
'component': string
'db.instance': string
Copy link
Contributor

Choose a reason for hiding this comment

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

(thanks for alphabetizing)

Copy link
Contributor

@bhs bhs left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like to wait at least 48h for others since it's the weekend.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM

@objectiser
Copy link
Contributor Author

@bhs Guess this can be merged?

@bhs
Copy link
Contributor

bhs commented Mar 8, 2017

Agreed, thanks again! Crazy day, sorry for delay.

@krisdwang
Copy link

How the spec think about DelayQueue scenarios or some message that never be consumed on time or some exception occurred when the message is consuming. I mean how to adjust if the span is closed or not correctly if use "follows-from" semantics?

@objectiser objectiser deleted the mom branch September 25, 2018 09:14
@objectiser
Copy link
Contributor Author

@krisdwang Not sure what you mean by "how to adjust if the span is closed"? Which span?

The message producer will result in a span being created (with kind producer) representing the scope of the message send/publish.

If one or more message consumers receive that message, then each of them will create a span (with kind consumer).

If the message expires and is never delivered to a consumer, then that will not be recorded - unless the message broker is instrumented and creates it's own span with a relevant tag/log.

If consuming the message results in an exception, then as long as that error occurs within the scope of the handler that has created the consumer span, then the error can be recorded against that span.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants