Skip to content

Add EnvironmentCarrier for propagating trace information across processes#2931

Closed
moskyb wants to merge 2 commits intoopen-telemetry:mainfrom
moskyb:main
Closed

Add EnvironmentCarrier for propagating trace information across processes#2931
moskyb wants to merge 2 commits intoopen-telemetry:mainfrom
moskyb:main

Conversation

@moskyb
Copy link
Copy Markdown

@moskyb moskyb commented May 30, 2022

This PR: Adds a default TextMapCarrier to the propagation package, EnvironmentCarrier. The purpose of this carrier is to propagate trace information cross-process, most notably in the case of tracing CI runs.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented May 30, 2022

CLA Not Signed

@hanyuancheung
Copy link
Copy Markdown
Member

Pls don't forget to sign the CLA.

Comment thread propagation/propagation.go Outdated
keys = append(keys, key)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

invalid blank line. Pls remove it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

will do!


// EnvironmentCarrier is a TextMapCarrier that uses the process environment as a
// storage medium for propagated key/value pairs
type EnvironmentCarrier struct{}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Setting environment variables with nothing prepended to them is quite unsafe (somebody could override PATH by mistake for example).
How about setting up a namespace? OTEL_CARRIER for example?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yep, that makes sense to me. so long as it's transparent to the user (ie, we add it before setting an envar and remove it before presenting it to the user), that's totally fine by me

@dmathieu
Copy link
Copy Markdown
Member

While the idea is interesting, I think this would need to go through an otep to have a formal specification, so it can be implemented the same way in every library.

@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented May 31, 2022

Thanks for your interest in the project. I do not think using globally defined environment variables to communicate intra-process cross-cutting concerns will work outside of the simplest of use cases. Regardless of that, @damemi correctly points out that this addition will require compatibility across OTel language implementations and therefor will require an OTEP and eventually a specification addition before we would be able to accept this change.

I'm going to close this based on this pre-mature state of the concept. Please feel free to re-submit when there is consensus for this addition from the specification.

@MrAlias MrAlias closed this May 31, 2022
@moskyb
Copy link
Copy Markdown
Author

moskyb commented May 31, 2022

I do not think using globally defined environment variables to communicate intra-process cross-cutting concerns will work outside of the simplest of use cases

@MrAlias can you elaborate on this a bit? the key usecase for me is tracing CI runs, where in some cases, the only way to communicate with a job being run is by environment variable - for example, if we were tracing our tests, there isn't really a place for the tests to get information about what ran them other than the process environment.

For full disclosure, i work at Buildkite, a CI provider, and a feature we're working on at the moment is OpenTelemetry support. I really think that this could be useful across the CI landscape though, and useful in general when one process is starting other processes, and we want to propagate traces across them

@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented May 31, 2022

I do not think using globally defined environment variables to communicate intra-process cross-cutting concerns will work outside of the simplest of use cases

@MrAlias can you elaborate on this a bit? the key usecase for me is tracing CI runs, where in some cases, the only way to communicate with a job being run is by environment variable - for example, if we were tracing our tests, there isn't really a place for the tests to get information about what ran them other than the process environment.

For full disclosure, i work at Buildkite, a CI provider, and a feature we're working on at the moment is OpenTelemetry support. I really think that this could be useful across the CI landscape though, and useful in general when one process is starting other processes, and we want to propagate traces across them

You will be limited by the unique nature of environment variable. It is not possible to communicate multiple cross-cutting concerns across multiple propagation pathways using this mechanism, at most it can hold the state for one.

If this solves your issues though, I would encourage you to use it. We designed the TextMapCarrier interface to be extensible for just such experimentation and extension. If you do find large community adoption, please feel free to submit the idea as an OTEP for ratification into an OTel standard.

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.

4 participants