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

Bug 1634064 - Implement the 'X-Source-Tags' header #1074

Merged
merged 9 commits into from
Jul 21, 2020

Conversation

Dexterp37
Copy link
Contributor

This implements the new ping tagging system in the Glean SDK. It adds a CLI option for adb, all the glean-core implementation and environment variable support.

Please note that I had to refactor the debug options module a bit in order to allow using non-String option types.

Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Not giving the green light just yet.
Code-wise this looks mostly good.

I did leave a couple of questions and suggestions inline. IMO we should not break consumers right away.

docs/user/debugging/index.md Outdated Show resolved Hide resolved
docs/user/debugging/android.md Outdated Show resolved Hide resolved
docs/user/debugging/android.md Outdated Show resolved Hide resolved
glean-core/src/debug.rs Outdated Show resolved Hide resolved
glean-core/src/debug.rs Outdated Show resolved Hide resolved
glean-core/src/debug.rs Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
docs/user/debugging/android.md Outdated Show resolved Hide resolved
docs/user/debugging/index.md Show resolved Hide resolved
docs/user/debugging/ios.md Outdated Show resolved Hide resolved
glean-core/src/debug.rs Outdated Show resolved Hide resolved
glean-core/src/debug.rs Outdated Show resolved Hide resolved
glean-core/src/debug.rs Outdated Show resolved Hide resolved
Before this PR, validation functions were also performing
the parsing of the data from environment variables. Unfortunately,
by being entangled, it was hard to provide options that were not
of type `String`. This PR fixes the problem by introducing an
`extraction` function: when instantiating a new option, we should
define how we want the data to be extracted from environment
variables.
This further decouples validation from extraction by
making sure the validation function only returns
true or false, instead of the validated value.
This option allows specifying a list of tags to be attached
to the ping upload requests, in order for the data users or
pipeline to tell data apart.
This adds the FFI boilerplate and changes the DebugActivity
in order to support the new option. Other language bindings,
except for Swift, will be supported using the environment
variables.
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Looking good, minimal nits and fixing the clippy issues

glean-core/src/debug.rs Show resolved Hide resolved
glean-core/src/debug.rs Show resolved Hide resolved
glean-core/src/debug.rs Show resolved Hide resolved
glean-core/src/debug.rs Show resolved Hide resolved
@Dexterp37 Dexterp37 merged commit 4538c83 into mozilla:main Jul 21, 2020
@Dexterp37 Dexterp37 deleted the source_tags branch July 21, 2020 09:01
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