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

Add support for Tag metadata #1125

Merged
merged 7 commits into from
Apr 23, 2019
Merged

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Apr 23, 2019

Replaces #1032

@rghetia rghetia requested review from rakyll and a team as code owners April 23, 2019 05:38
@@ -95,13 +100,14 @@ type Mutator interface {
// Insert returns a mutator that inserts a
// value associated with k. If k already exists in the tag map,
// mutator doesn't update the value.
func Insert(k Key, v string) Mutator {
// Metadata applies metadata to the tag. It is optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there're more than one metadatas and they're conflicting? E.g Insert(k, v, unlimitedPropagation, noPropagation)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the last one will be effective.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a note on the API document, and adding a few tests on this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a note in the apis and testcases.

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 overall

@@ -95,13 +100,14 @@ type Mutator interface {
// Insert returns a mutator that inserts a
// value associated with k. If k already exists in the tag map,
// mutator doesn't update the value.
func Insert(k Key, v string) Mutator {
// Metadata applies metadata to the tag. It is optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a note on the API document, and adding a few tests on this case.

@rghetia rghetia merged commit 917e071 into census-instrumentation:dev Apr 23, 2019
@rghetia rghetia deleted the tag_metadata branch April 23, 2019 22:18
rghetia added a commit to rghetia/opencensus-go that referenced this pull request Apr 25, 2019
* Add support for tag metadata.

* update ocgrpc and ochttp to use new insert/update/upsert api.
:

* updated existing method optional metadata option.

* make TTLNoPropagation and TTLUnlimitedPropagation a function.

* changed ttl api.

* add test case for multiple TTL metadata.

* add test case and note for update/insert api.
rghetia added a commit that referenced this pull request Apr 25, 2019
* Add support for tag metadata.

* update ocgrpc and ochttp to use new insert/update/upsert api.
:

* updated existing method optional metadata option.

* make TTLNoPropagation and TTLUnlimitedPropagation a function.

* changed ttl api.

* add test case for multiple TTL metadata.

* add test case and note for update/insert api.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants