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 clarification about async vs. sync instruments #1733

Merged

Conversation

reyang
Copy link
Member

@reyang reyang commented May 28, 2021

Changes

  • Editorial change - added clarification and examples about sync vs. async instruments.

Related oteps OTEP146

@reyang reyang requested review from a team May 28, 2021 22:57
@reyang reyang added area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory labels May 28, 2021

<a name="synchronous-instrument"></a>

* Synchronous instruments (e.g. [Counter](#counter)) are meant to be invoked
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: meant to be invoked inline with application/business processing logic, within the scope of [Context](../context/context.md).

Baggage (as specififed) is available via a context. I think you can call it out below as an additional thing, but the key here is;

  • The logic is inline with the mainline processing of the application (not sheddable when under load)
  • The logic can have automagical attachment of context if desired (and all the fun contexty things).

Copy link
Member Author

Choose a reason for hiding this comment

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

Please take another look, I believe this has been addressed.

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

👍🏼

@Oberon00 Oberon00 mentioned this pull request Jun 4, 2021
@carlosalberto
Copy link
Contributor

Let's merge this on Monday prior to the 1.4.0 release.

Instruments can be categorized based on whether they are synchronous or
asynchronous:

<a name="synchronous-instrument"></a>
Copy link
Member

Choose a reason for hiding this comment

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

Why html tags like this? Should use markdown headers if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @reyang

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the same as other tags in this doc (and other spec doc such as the tracing one):

  1. we don't want them to be in the top level ToC (since they interrupt the ToC flow)
  2. we want to have an anchor

IIRC @bogdandrutu you've asked the same question before?
e.g. https:/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-naming-rule

#1578 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@bogdandrutu Please fill an issue to follow up this if this is still a problem, etc. Thanks!

@carlosalberto carlosalberto merged commit ec88c03 into open-telemetry:main Jun 7, 2021
@reyang reyang deleted the reyang/clarify-async-sync-instruments branch October 4, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants