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 support for environment and dataset rules with same names #438

Merged
merged 6 commits into from
Apr 26, 2022

Conversation

MikeGoldsmith
Copy link
Contributor

Which problem is this PR solving?

Adds support for config files to contain both a dataset and environment with the same name in the same rules config. This is achieved by adding a DatasetPrefix to the config and then using that prefix when setting up rule definitions.

config.toml

DatasetPrefix = "dataset"

rules.toml

Sampler = "DeterministicSampler"
SampleRate = 1

[production] # environment
  Sampler = "DeterministicSampler"
  SampleRate = 10

[dataset.production] # dataset
  Sampler = "DeterministicSampler"
  SampleRate = 20

Short description of the changes

  • Adds new config option DatasetPrefix, defaults to ""
  • If a legacy is provided to the SamplerFactory, it uses the configured prefix along with the sample key to locate rules definitions
  • Adds test to verify behaviour

@MikeGoldsmith MikeGoldsmith added type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Apr 20, 2022
@MikeGoldsmith MikeGoldsmith self-assigned this Apr 20, 2022
@MikeGoldsmith MikeGoldsmith requested a review from a team April 20, 2022 14:29
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

Generally approved, but two notes:

  • I know there's probably going to be a docs-related PR for this as well, but I think it also deserves a note in the README.
  • It took me a while to understand the description of this PR. Can you please add a bit of a note as to why there's a problem that needs this fix?

collect/collect.go Outdated Show resolved Hide resolved
config/file_config.go Outdated Show resolved Hide resolved
sample/sample.go Outdated Show resolved Hide resolved
Comment on lines +29 to +30
if prefix := s.Config.GetDatasetPrefix(); prefix != "" {
samplerKey = fmt.Sprintf("%s.%s", prefix, samplerKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could mandate that the intervening period be part of the specified prefix. If I set the DatasetPrefix field to "x," the prefix really becomes "x."—even though I didn't mention the period when specifying my preferred prefix. I might prefer a prefix like "dataset-" (with a separating hyphen instead of a period).

Had you considered including this separator as part of the prefix? Were you attempting to avoid false aliasing if there was no obvious separator character included in the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree additional docs on the prefix format where the . separator is added for you would be useful 👍🏻

The separator is not necessary but I think helps create a mental divide. The intent of this change is to support users migrate from legacy datasets to newer environment and services rule sets and should not be a permanent solution. I think a simple solution of always using a single . that's well documented should be good enough.

@MikeGoldsmith MikeGoldsmith requested a review from kentquirk April 25, 2022 13:54
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

Thanks for the docs improvement. I added suggestions for a couple of typos.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Kent Quirk <kentquirk@gmail.com>
@MikeGoldsmith MikeGoldsmith merged commit 9153097 into main Apr 26, 2022
@MikeGoldsmith MikeGoldsmith deleted the mike/dataset-prefix branch April 26, 2022 19:26
ghost pushed a commit to opsramp/tracing-proxy that referenced this pull request Jul 5, 2024
…ombio#438)

Co-authored-by: Steven E. Harris <seh@panix.com>
Co-authored-by: Kent Quirk <kentquirk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add default environment prefix to allow environments and dataset with same name in rules file
3 participants