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

add constant labels to gauges and cumulative metrics#1122

Merged
rghetia merged 4 commits intocensus-instrumentation:devfrom
paivagustavo:gauge-const-labels-no-force-push
Apr 23, 2019
Merged

add constant labels to gauges and cumulative metrics#1122
rghetia merged 4 commits intocensus-instrumentation:devfrom
paivagustavo:gauge-const-labels-no-force-push

Conversation

@paivagustavo
Copy link
Copy Markdown
Contributor

@paivagustavo paivagustavo commented Apr 21, 2019

This PR adds support for constant labels on gauges.

The proposed syntax is:

r := NewRegistry()

 	f, _ := r.AddFloat64Gauge("TestGaugeWithConstLabel",
		WithLabelKeys("k1"), WithConstLabel("const", "same"), WithConstLabel("const2", "same2"))

@rghetia this is my first contribution and as you were the one who created the issue, could you review this?

closes #1112

@paivagustavo paivagustavo requested review from a team, rakyll and rghetia as code owners April 21, 2019 04:08
@paivagustavo paivagustavo mentioned this pull request Apr 21, 2019
Closed
rghetia
rghetia reviewed Apr 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

@paivagustavo thanks for the PR.

we are making commits to dev branch for now. I'll merge the dev branch to master just before the next release.
So can you please rebase your change to dev?

When you rebase, you will have conflict as I have created LabelKey type which contains key and description. So you will have to change that accordingly.

@paivagustavo paivagustavo changed the base branch from master to dev April 23, 2019 02:01
@paivagustavo
Copy link
Copy Markdown
Contributor Author

paivagustavo commented Apr 23, 2019

@rghetia I've rebased to dev and change the target branch (I may have pick some commits from the master that was not on dev yet, is it a problem?)

While doing this, I noticed that the canonicalize was not deterministic and sometimes the TestGauge was failing, I have fixed this and made it more reliable while keeping the same purpose.

@rghetia
Copy link
Copy Markdown
Contributor

rghetia commented Apr 23, 2019

@rghetia I've rebased to dev and change the target branch (I may have pick some commits from the master that was not on dev yet, is it a problem?)

yes, that will be a problem. you can do git rebase --onto dev HEAD~2 since you two commits.

@paivagustavo paivagustavo changed the title add constant labels to gauges add constant labels to gauges and cumulative metrics Apr 23, 2019
@paivagustavo
Copy link
Copy Markdown
Contributor Author

paivagustavo commented Apr 23, 2019

Done, the PR now contains only the correct commits.

rghetia
rghetia approved these changes Apr 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

one minor nit. Otherwise LGTM.

@rghetia rghetia merged commit 8986fa9 into census-instrumentation:dev Apr 23, 2019
@rghetia rghetia mentioned this pull request Apr 23, 2019
Closed
rghetia pushed a commit to rghetia/opencensus-go that referenced this pull request Apr 25, 2019
@paivagustavo @rghetia
add constant labels to gauges and cumulative metrics (census-instrume…
15a5327 
…ntation#1122)

* Remove unused GetEntry.

* adds support for constant labels on Gauge and CumulativeMetric

* fixing format on tests.

* remove unused getentry
rghetia pushed a commit that referenced this pull request Apr 25, 2019
@paivagustavo @rghetia
add constant labels to gauges and cumulative metrics (#1122)
c31d268 
* Remove unused GetEntry.

* adds support for constant labels on Gauge and CumulativeMetric

* fixing format on tests.

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

Reviewers

@rakyll rakyll Awaiting requested review from rakyll

1 more reviewer

@rghetia rghetia rghetia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@paivagustavo @rghetia