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 'meta.refinery.local_hostname' to all spans #250

Merged
merged 2 commits into from
May 12, 2021

Conversation

jharley
Copy link
Contributor

@jharley jharley commented May 6, 2021

The intention is to help determine issues with a single Refinery instance.

@jharley jharley added status: review needed Changes need review. version: bump minor A PR that adds behavior, but is backwards-compatible. labels May 6, 2021
@jharley jharley requested a review from a team May 6, 2021 13:15
@vreynolds
Copy link
Contributor

Hurm. I tried it out locally, and I'm not seeing the extra metadata on the events. I think the AddField needs to happen before we grab the builder?

@jharley
Copy link
Contributor Author

jharley commented May 7, 2021

Urgh, thank you and my apologies! :\

Adding the field before the builder is created (or on the builder directly) does give the desired result, but it breaks the testsuite as the field is being added all the event data.

If you feel this is useful -- I think it's a helpful addition -- perhaps it should be a config option anyway? Then it can be disabled for the testsuite to avoid having to add the field to all the expected test results. Perhaps something like Config.GetEnableEventHostMetadata?

@vreynolds
Copy link
Contributor

No worries! It wasn't intuitive to me, either.

I like the idea of adding a config option, because there's already something akin to it: AddSampleRateKeyToTrace in the rules config, except this new one (maybe something like AddHostMetadataToTrace = true/false) would be in the general config. That way, a single test that flips that to true can test the hostname, and all other tests should work the same.

Does that make sense?

@jharley
Copy link
Contributor Author

jharley commented May 7, 2021

Makes good sense to me! I'll pick that up and run with it. 🚀

@jharley jharley added status: revision needed Waiting for response to changes requested. and removed status: review needed Changes need review. labels May 7, 2021
@jharley jharley marked this pull request as draft May 7, 2021 17:11
Currently just adding 'meta.refinery.local_hostname' to all spans.

The intention with this (and any additional) field is to assist in
troubleshooting issues with a single Refinery instance.
@jharley jharley force-pushed the refinery-meta-hostname branch from 1744d72 to d798009 Compare May 8, 2021 15:11
@jharley jharley added status: review needed Changes need review. and removed status: revision needed Waiting for response to changes requested. labels May 8, 2021
@jharley jharley marked this pull request as ready for review May 8, 2021 15:14
Copy link
Contributor

@vreynolds vreynolds left a comment

Choose a reason for hiding this comment

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

Question about the default value, and small update about live reload, otherwise LGTM!

config_complete.toml Outdated Show resolved Hide resolved
@jharley jharley requested a review from vreynolds May 12, 2021 18:38
@vreynolds vreynolds merged commit 7d3b4a4 into honeycombio:main May 12, 2021
@vreynolds
Copy link
Contributor

@jharley jharley deleted the refinery-meta-hostname branch May 12, 2021 19:23
@jharley
Copy link
Contributor Author

jharley commented May 12, 2021

@jharley could you update the docs as well? https://docs.honeycomb.io/manage-data-volume/refinery/configuration/#general-configuration

No problem!

ghost pushed a commit to opsramp/tracing-proxy that referenced this pull request Jul 5, 2024
New config option AddHostMetadataToTrace

Currently just adding 'meta.refinery.local_hostname' to all spans.

The intention with this (and any additional) field is to assist in
troubleshooting issues with a single Refinery instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: review needed Changes need review. 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.

2 participants