Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

Recording http metrics at host level#1926

Merged
songy23 merged 4 commits intocensus-instrumentation:masterfrom
iamgr0ot:kavi/add-host-tag
Jun 10, 2019
Merged

Recording http metrics at host level#1926
songy23 merged 4 commits intocensus-instrumentation:masterfrom
iamgr0ot:kavi/add-host-tag

Conversation

@iamgr0ot
Copy link
Copy Markdown
Contributor

@iamgr0ot iamgr0ot commented Jun 7, 2019

Host name level opencensus metrics are insanely useful while troubleshooting production issues and we would like to give it back to the community.

  • Recording opencensus client http metrics at hostname level based on configuration.
  • Introducing HttpClientMetricGranularity to define, at what level, one would like to collect metrics.
  • The change is backward compatible and metrics will be collected at http method level by default.

@iamgr0ot iamgr0ot requested review from a team, dinooliva, rghetia and songy23 as code owners June 7, 2019 21:59
@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@bogdandrutu
Copy link
Copy Markdown
Contributor

Can you please sign the CLA?

package io.opencensus.contrib.http.util;

/** Enum class that determines the granularity at which opencensus http metrics are emitted. */
public enum HttpClientMetricGranularity {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure why do you need this? What you can do is to always add the HOST tag, but don't change the default views. So you can define your own custom views that include the host :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what if some applications have a high cardinality hostname space, for example web crawlers? Metric backends may run into cardinality issues in that case. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the defined views do not include the HOST we just ignore the tag so no cardinality issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't quite get your comment on custom views.

@iamgr0ot
Copy link
Copy Markdown
Contributor Author

iamgr0ot commented Jun 7, 2019

I signed it!

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 7, 2019
@iamgr0ot
Copy link
Copy Markdown
Contributor Author

iamgr0ot commented Jun 7, 2019

@bogdandrutu - You are right. There is no need to change default views. I could define my own view with the exposed measure. Now, I am just tagging host name and not changing any default views.

Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

OC-Specs also specified that default HTTP views don't have the host key. Though it definitely should be in the context that measurements are recorded against.

@songy23 songy23 merged commit 2375b22 into census-instrumentation:master Jun 10, 2019
@iamgr0ot
Copy link
Copy Markdown
Contributor Author

@songy23 - I would love to get this change with a release. Could you please help me with that?

@songy23
Copy link
Copy Markdown
Contributor

songy23 commented Jun 11, 2019

@iamgr0ot We can do a new release after finishing the remaining HTTP work - @rghetia do you have an estimate on it?

EDIT: sorry for mentioning the wrong username

@rghetia
Copy link
Copy Markdown
Contributor

rghetia commented Jun 11, 2019

I have two PRs pending. I will post them today. we can release later today or tomorrow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants