Skip to content

Add a small explanation on why Restore is needed. #563

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

Merged

Conversation

carlosalberto
Copy link
Contributor

Fixes #490

cc @toumorokoshi

This operation is intended to help making sure the correct `Context`
is associated with the caller's current execution unit. Users can
rely on it to identify a wrong call order and emit a signal to warn
users about the broken invariant.
Copy link
Member

Choose a reason for hiding this comment

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

Users can rely on it to identify a wrong call order and emit a signal to warn users about the broken invariant.

Is a bit unclear to me. I'm reading between the lines, but guessing:

Wrong call order means:

because there is a pattern that an attach of a context should be paired with a detach, then someone reading the code will be able to spot the lifetime of the context. In this regard, someone can quickly identify if the context is detached in the wrong location

"emit a signal to warn users about the broken invariant" I'm having a hard time guessing. What signal is emitted? I'm assuming "broken invariant" is referring to the context object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@toumorokoshi Updated it to make it a little bit clearer: so basically

  1. The wrong call order: trying the incorrect Context (usually not the current instance)
  2. Emit a signal: either logging an error or return a value (this is implementation specific).

Let me know ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping @toumorokoshi Let me me know if this is better now ;)

Copy link
Member

Choose a reason for hiding this comment

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

I think the examples actually help the most. But either way good on merging it and not waiting for my very slow feedback :)

@carlosalberto
Copy link
Contributor Author

@arminru Added a small note on the optional return value of the Detach Context operation. Wondering if this is enough? I know most implementations (all?) return nothing, but this way we can leave some space open for future additions ;)

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@bogdandrutu bogdandrutu merged commit c03fe50 into open-telemetry:master Apr 28, 2020
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* Add a small explanation on why Restore is needed.

* Add a small note on the return value.

* Clarify more.

* Update specification/context/context.md

Co-Authored-By: Armin Ruech <[email protected]>

Co-authored-by: Armin Ruech <[email protected]>
Co-authored-by: Bogdan Drutu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an explanation the need for Context's Restore operation.
6 participants