Add supports for printing pretty histogram to stdout #1028

mujtaba-ahmed12 · 2020-08-04T18:19:14Z

No description provided.

jmacd

Please revert the unrelated go.mod and go.sum changes.

mujtaba-ahmed12 · 2020-08-05T19:03:40Z

Please revert the unrelated go.mod and go.sum changes.

This is happening make precommit command.

MrAlias · 2020-08-05T22:53:47Z

Please revert the unrelated go.mod and go.sum changes.

This is happening make precommit command.

The build is failing on go.sums not being up to date which would imply it was not run before the commit was made.

Are you sure you ran make precomit prior to the commit? Or, maybe the fix didn't get pushed?

Regardless of the validity of the module hashes, what @jmacd is pointing out is a lot of the changes to the go.sum files is from the modification to the go.mod files. These changes to the go.mod files look to be in error. They are unrelated to any changes in the code. The only code change in the PR adds an additional import of the standard math package. That is not what is being changed in all of the go.mod or go.sum files.

Please revert the changes to all go.mod files and sync the go.sum files. This can be accomplished by running make in the top of the package once the changes to go.mod files are reverted.

MrAlias · 2020-08-06T03:50:30Z

Took a deeper look into the go.mod modifications. It looks like there is some cleanup that can be done in the main modules that will remove a lot of indirect dependencies because of our cleanup efforts in other PRs. I'm going to wait on #1038 which will pull out another dependency before submitting a PR to do a deep clean.

That said, I'm still at a loss why your local system is making the changes it is. These are no automatically made by the local build system for me when I do not prune anything manually. It seems like my advice above about reverting changes to all go.mod files will help clean this up if you don't want to wait for my eventual "deep clean" PR.

jmacd · 2020-08-06T06:48:00Z

I fell that I've seen this sort of problem arise as follows:

work on a refactoring, e.g., change package names, do something so that go tries to resolve a non-existent package
before failing to resolve a non-existent package, it will try to update the go.mod
you fix the code because there is actually supposed to not be a package, and the go.mod changes are still there

I would basically revert the change and ignore it, since it seemed accidental.

jmacd · 2020-08-11T05:12:47Z

exporters/stdout/metric.go

@@ -44,10 +45,22 @@ type line struct {

 	Quantiles []quantile `json:"Quantiles,omitempty"`

+	Histogram []bucket `json:"histogram,omitempty"`


I see below a switch on e.config.PrettyPrint which directly calls Fprintf, and I wonder if the structure would be more readable if the pretty-form of histogram printed in-line with the JSON, formatted within quotes, like:

... Histogram: " _pretty representation_ ... "

This could be implemented using a new type with a MarshalJSON() method that switched on e.config.PrettyPrint itself. How does this sound?

MrAlias · 2020-08-27T19:52:09Z

exporters/stdout/metric.go

@@ -44,13 +45,37 @@ type line struct {

 	Quantiles []quantile `json:"Quantiles,omitempty"`

+	Histogram histogram `json:"histogram,omitempty"`


Can we follow the other naming structure here and make this capital.

Suggested change

Histogram histogram `json:"histogram,omitempty"`

Histogram histogram `json:"Histogram,omitempty"`

MrAlias · 2020-08-27T19:52:34Z

exporters/stdout/metric.go

+	boundary struct {
+		Min float64 `json:"min"`
+		Max float64 `json:"max"`
+	}
+
+	bucket struct {
+		boundary `json:"bounds"`
+		Count    float64 `json:"count"`


Similar here:

Suggested change

boundary struct {

Min float64 `json:"min"`

Max float64 `json:"max"`

}

bucket struct {

boundary `json:"bounds"`

Count float64 `json:"count"`

boundary struct {

Min float64 `json:"Min"`

Max float64 `json:"Max"`

}

bucket struct {

boundary `json:"Bounds"`

Count float64 `json:"Count"`

MrAlias · 2020-08-27T19:54:14Z

exporters/stdout/metric.go

+	bytes, err := json.Marshal(hist)
+	if err != nil {
+		return nil, err
+	}
+	return append(bytes, []byte("\n"+printHistogram(hist))...), nil


I'm not following why we are Marshaling with json and printing the histogram. This is redundantly showing the same information.

MrAlias · 2020-08-27T19:56:43Z

exporters/stdout/metric.go

+					return err
+				}
+				buckets := make([]bucket, 0, len(val.Boundaries))
+				lowerBound := float64(-math.MaxFloat64)


Shouldn't this be negative infinity? Or is there a limitation here?

Suggested change

lowerBound := float64(-math.MaxFloat64)

lowerBound := math.Inf(-1)

MrAlias · 2020-08-27T19:57:22Z

exporters/stdout/metric.go

+					})
+					lowerBound = val.Boundaries[i]
+					if i == len(val.Boundaries)-1 {
+						upperBound = float64(math.MaxFloat64)


Suggested change

upperBound = float64(math.MaxFloat64)

upperBound = math.Inf(1)

MrAlias · 2020-08-27T19:59:02Z

exporters/stdout/metric.go

+)
+
+func (hist histogram) MarshalJSON() ([]byte, error) {
+	bytes, err := json.Marshal(hist)


Can we have the pretty print just return the similarly formatted JSON structure like the other formats and have the default return the tabular form? That would provide the expected consistency.

MrAlias · 2020-10-01T17:13:33Z

@evantorrie is going to take this over.

MrAlias · 2020-11-19T21:49:57Z

Closing due to staleness.

mujtaba-ahmed12 requested review from Aneurysm9, evantorrie, jmacd, lizthegrey and MrAlias as code owners August 4, 2020 18:19

jmacd reviewed Aug 5, 2020

View reviewed changes

mujtaba-ahmed12 force-pushed the stdout-histogram branch from 47bbbbb to 18e3f14 Compare August 5, 2020 19:02

Add supports for printing pretty histogram to stdout

5091a30

mujtaba-ahmed12 force-pushed the stdout-histogram branch from 18e3f14 to 5091a30 Compare August 7, 2020 19:00

mujtaba-ahmed12 requested a review from jmacd August 7, 2020 19:00

jmacd reviewed Aug 11, 2020

View reviewed changes

jmacd added the area:metrics Part of OpenTelemetry Metrics label Aug 11, 2020

Override MarshalJSON method for histogram

4aefa60

mujtaba-ahmed12 requested a review from jmacd August 12, 2020 21:48

MrAlias linked an issue Aug 20, 2020 that may be closed by this pull request

Add Histogram Support to STDOUT Exporter #954

Closed

MrAlias added the release:after-ga label Aug 27, 2020

MrAlias reviewed Aug 27, 2020

View reviewed changes

MrAlias removed a link to an issue Oct 1, 2020

Add Histogram Support to STDOUT Exporter #954

Closed

MrAlias closed this Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add supports for printing pretty histogram to stdout #1028

Add supports for printing pretty histogram to stdout #1028

mujtaba-ahmed12 commented Aug 4, 2020

jmacd left a comment

mujtaba-ahmed12 commented Aug 5, 2020

MrAlias commented Aug 5, 2020

MrAlias commented Aug 6, 2020

jmacd commented Aug 6, 2020

jmacd Aug 11, 2020

MrAlias Aug 27, 2020

MrAlias Aug 27, 2020

MrAlias Aug 27, 2020

MrAlias Aug 27, 2020

MrAlias Aug 27, 2020

MrAlias Aug 27, 2020

MrAlias commented Oct 1, 2020

MrAlias commented Nov 19, 2020

		@@ -44,10 +45,22 @@ type line struct {

		Quantiles []quantile `json:"Quantiles,omitempty"`

		Histogram []bucket `json:"histogram,omitempty"`

		@@ -44,13 +45,37 @@ type line struct {

		Quantiles []quantile `json:"Quantiles,omitempty"`

		Histogram histogram `json:"histogram,omitempty"`

	lowerBound := float64(-math.MaxFloat64)
	lowerBound := math.Inf(-1)

	upperBound = float64(math.MaxFloat64)
	upperBound = math.Inf(1)

Add supports for printing pretty histogram to stdout #1028

Add supports for printing pretty histogram to stdout #1028

Conversation

mujtaba-ahmed12 commented Aug 4, 2020

jmacd left a comment

Choose a reason for hiding this comment

mujtaba-ahmed12 commented Aug 5, 2020

MrAlias commented Aug 5, 2020

MrAlias commented Aug 6, 2020

jmacd commented Aug 6, 2020

jmacd Aug 11, 2020

Choose a reason for hiding this comment

MrAlias Aug 27, 2020

Choose a reason for hiding this comment

MrAlias Aug 27, 2020

Choose a reason for hiding this comment

MrAlias Aug 27, 2020

Choose a reason for hiding this comment

MrAlias Aug 27, 2020

Choose a reason for hiding this comment

MrAlias Aug 27, 2020

Choose a reason for hiding this comment

MrAlias Aug 27, 2020

Choose a reason for hiding this comment

MrAlias commented Oct 1, 2020

MrAlias commented Nov 19, 2020