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

Batch time series with different label values into same request. #119

Merged
merged 2 commits into from
Apr 1, 2019

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Apr 1, 2019

fixes #120

@rghetia rghetia requested review from odeke-em and songy23 April 1, 2019 20:28
Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @rghetia! LGTM with some suggestions but also it would be great to file
a bug describing the problem and also what the fix is. This will massively help in bug diagnosis
and regression analysis to figure out why we added this code; just like you saw how we debugged
and came up with the solution. Once you have opened the bug, please refer to it in the comments
here and also reference it in the commit message.

metrics_proto.go Outdated
@@ -39,6 +39,8 @@ import (
commonpb "github.com/census-instrumentation/opencensus-proto/gen-go/agent/common/v1"
metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
resourcepb "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"
"sort"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put these imports in the same section as the standard library imports.
i.e.

import (
	"context"
	"errors"
	"fmt"
	"path"
        "sort"
        "strings"

metrics_proto.go Outdated
@@ -117,6 +119,17 @@ func (se *statsExporter) handleMetricsProtoUpload(payloads []*metricProtoPayload
return nil
}

func signature(metric *googlemetricpb.Metric) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps let's add a comment here and also rename this function:

// metricSignature creates a unique signature consisting of a
// metric's type and its lexicographically sorted label values
func metricSignature(metric *googlemetricpb.Metric) string {

Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Still LGTM, with the last suggestion!

@@ -117,6 +119,19 @@ func (se *statsExporter) handleMetricsProtoUpload(payloads []*metricProtoPayload
return nil
}

// metricSignature creates a unique signature consisting of a
// metric's type and its lexicographically sorted label values
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one last comment request. Since you've created the issue, please add it here to finally make this comment:

// metricSignature creates a unique signature consisting of a
// metric's type and its lexicographically sorted label values.
// See https://github.com/census-ecosystem/opencensus-go-exporter-stackdriver/issues/120
func metricSignature(metric *googlemetricpb.Metric) string {

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

@rghetia rghetia merged commit dc2fe43 into census-ecosystem:master Apr 1, 2019
@rghetia rghetia deleted the batching branch April 18, 2019 04:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timeseries with same type but different labels are not batched.
4 participants