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 local instance's tags to forwarded metrics #188

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cory-stripe
Copy link
Contributor

@cory-stripe cory-stripe commented Jun 30, 2017

Summary

Apply a local veneur's configured tags to metrics that are forwarded.

Motivation

We were asked to do this in #153. It was perhaps an oversight / bug that a local instance's tags are not passed along to the importer.

The idea is that if my local veneur has a tag like host_type then the importer will receive it. Today, as far as I can tell, this doesn't happen. We just inherit the tags of the importer at the time it flushes to the sink.

Test plan

Added a new unit test, as well as some plumbing that allows future-us to more easily leverage the fixtures HTTP test harness to deal with /import calls.

r? @aditya-stripe

@cory-stripe cory-stripe changed the title WIP Add local instance's tags to forwarded metrics Add local instance's tags to forwarded metrics Jul 3, 2017
@stripe-ci
Copy link

Gerald Rule: Copy Observability on Veneur and Unilog pull requests

cc @stripe/observability
cc @stripe/observability-stripe

@cory-stripe cory-stripe closed this Jul 5, 2017
@cory-stripe cory-stripe deleted the cory-tag-forwarded-metrics branch July 5, 2017 15:50
@cory-stripe cory-stripe restored the cory-tag-forwarded-metrics branch July 5, 2017 15:55
@cory-stripe cory-stripe reopened this Jul 5, 2017
@cory-stripe
Copy link
Contributor Author

cory-stripe commented Jul 5, 2017

(Oops, accidentally removed this when cleaning up branches.)

There are some repercussions to this change. Here's a sample of tags we add to our metrics:

tags:
  - 'host_type:splunkmaster'
  - 'host_domain:stripe.whatever'
  - 'host_lsbdistcodename:trusty'
  - 'host_contact:observability'
  - 'instance-type:m3.large'
  - 'availability-zone:us-west-2a'
  - 'host_env:qa'
  - 'host_cluster:northwest'

This could cause some sneaky tags to show and create multiple time series where they do not currently exist, some examples:

  • an upgraded box running xenial
  • mixed instance types
  • AZ

It seems annoying to filter out some of these but it's not necessarily intended to have this create a few time series from what is currently one aggregated one.

(In other words, every time series we aggregate would suddenly become multiple due to AZ being mixed in most host groups.)

@ChimeraCoder
Copy link
Contributor

This sounds like it would have a significant impact on the cardinality of some of our already-large metrics, right? Particularly stuff like unilog?

@cory-stripe
Copy link
Contributor Author

It would definitely have negative effects. But also sorta feels like a bug I think. As #153 makes clear at least some people expect this behavior. Do we not expect it because we're used to the bug?

We could also — and this feels icky — blacklist some tags from global proliferation? In other words, here are my tags but when you forward please exclude key foobar or something…

@ChimeraCoder
Copy link
Contributor

So, what we could do is ensure (in our deployment) that tags that are configured on the local host are also set on the global boxes as well (thereby overriding the local ones). That'd probably solve our cardinality concerns.

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