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

Add support for Tag metadata#1125

Merged
rghetia merged 7 commits intocensus-instrumentation:devfrom
rghetia:tag_metadata
Apr 23, 2019
Merged

Add support for Tag metadata#1125
rghetia merged 7 commits intocensus-instrumentation:devfrom
rghetia:tag_metadata

Conversation

@rghetia
Copy link
Copy Markdown
Contributor

@rghetia rghetia commented Apr 23, 2019

Replaces #1032

@rghetia rghetia requested review from a team and rakyll as code owners April 23, 2019 05:38
@rghetia rghetia requested review from bogdandrutu and songy23 April 23, 2019 05:40
// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

// 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
Copy Markdown
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