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

Add yaml for FaaS trace attributes #1054

Merged
merged 7 commits into from
Oct 19, 2020

Conversation

thisthat
Copy link
Member

@thisthat thisthat commented Oct 5, 2020

Fixes #547

Changes

Editorial change.
This PR introduces the YAML definitions of FaaS span attributes.

@thisthat thisthat requested review from a team October 5, 2020 06:51
@arminru arminru added area:semantic-conventions Related to semantic conventions release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs labels Oct 5, 2020
semantic_conventions/trace/faas.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/faas.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/faas.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/faas.yaml Outdated Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

I think we are overusing the numbered footnotes a bit. I think we can keep most of the text in the Description field. Use footnotes if the Description is too large and becomes a wall of text or when it is needed to describe a list of possible values as a separate table (I like that).

specification/trace/semantic_conventions/faas.md Outdated Show resolved Hide resolved
Comment on lines 39 to 43
| `datasource` | A response to some data source operation such as a database or filesystem read/write. |
| `http` | To provide an answer to an inbound HTTP request |
| `pubsub` | A function is set to be executed when messages are sent to a messaging system. |
| `timer` | A function is scheduled to be executed regularly. |
| `other` | If none of the other applies |
Copy link
Member

Choose a reason for hiding this comment

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

Did we have these descriptions in faas.md previously or these are new in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

The descriptions are new but only taken over and paraphrased from the first sentence of the respective section for each trigger type.

specification/trace/semantic_conventions/faas.md Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Oberon00 commented Oct 6, 2020

@tigrannajaryan

I think we are overusing the numbered footnotes a bit. I think we can keep most of the text in the Description field.

I agree that the current footnote/description split in the markdown looks often bad. But I think semantically the split is OK in the YAML. The brief: should be something that is usable in a Javadoc @brief for example, so it should be the bare minimum. Maybe we can tweak the markdown generator to append/merge the description to brief if the result is still reasonably short.

@arminru arminru requested review from tigrannajaryan and a team October 9, 2020 14:00
@thisthat
Copy link
Member Author

@tigrannajaryan could you please have another look? I reduced the footnotes as you suggested. However, I left the ones for the Outgoing Invocation. My reasoning for it is that they provide a weak constraint on the attribute and do not describe it.

- id: faas_span
prefix: faas
brief: >
This document defines how to describe an instance of a function that
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should not talk about "this document" in the YAML, as it may become javadoc or something else.

semantic_conventions/trace/faas.yaml Outdated Show resolved Hide resolved
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

@thisthat I'm ok to merge. If you want to change the "This document" comment, lmk

@thisthat
Copy link
Member Author

@SergeyKanzhelev I have addressed the comment :)

Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
@arminru arminru merged commit 04b2026 into open-telemetry:master Oct 19, 2020
@arminru arminru deleted the yaml-trace-faas branch October 19, 2020 16:36
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Define semantic conventions in JSON/YAML
5 participants