Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Add Measure, TagKey and View constants for recording HTTP stats #360

Merged

Conversation

mayurkale22
Copy link
Member

Copy link
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 except minor nits.

/cc @rghetia

};

const SIZE_DISTRIBUTION: number[] = [
0, 1024, 2048, 4096, 16384, 65536, 262144, 1048576, 4194304, 16777216,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we still want to include the leading 0s?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would keep it now, until we officially remove this from the specs.
Context (for the record) -> we are already dropping negative and zero bucket bounds in top level API, so it doesn't make sense to include 0 at first place. Also specs says "The values must be strictly increasing and > 0".

Copy link

Choose a reason for hiding this comment

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

Lets remove it. We can open another PR to update the specs. For now we have two conflicting specs (one for metrics proto and one for http/grpc stats). Since we don't have any useful data in zero bucket might as well remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done.


/** Register all default client views. */
export function registerAllClientViews(globalStats: Stats) {
for (const CLIENT_VIEW of HTTP_BASIC_CLIENT_VIEWS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: should we use ALL_CAPS for const declarations that are local to a function (here and for registerAllServerViews below)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

};

const SIZE_DISTRIBUTION: number[] = [
0, 1024, 2048, 4096, 16384, 65536, 262144, 1048576, 4194304, 16777216,
Copy link

Choose a reason for hiding this comment

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

Lets remove it. We can open another PR to update the specs. For now we have two conflicting specs (one for metrics proto and one for http/grpc stats). Since we don't have any useful data in zero bucket might as well remove it.

@codecov-io
Copy link

Codecov Report

Merging #360 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #360   +/-   ##
======================================
  Coverage    95.3%   95.3%           
======================================
  Files         124     124           
  Lines        8413    8413           
  Branches      624     624           
======================================
  Hits         8018    8018           
  Misses        395     395

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8572750...7845d97. Read the comment docs.

@mayurkale22 mayurkale22 merged commit 8ba7367 into census-instrumentation:master Feb 26, 2019
@mayurkale22 mayurkale22 deleted the http_stats_constats branch February 26, 2019 20:15
@mayurkale22 mayurkale22 added this to the Release 0.0.10 milestone Mar 8, 2019
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.

None yet

6 participants