Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

putAttribute vs. setAttribute #25

Closed
yurishkuro opened this issue Mar 28, 2019 · 11 comments
Closed

putAttribute vs. setAttribute #25

yurishkuro opened this issue Mar 28, 2019 · 11 comments
Labels
Agreed Issues that the group agreed on. Terminology alignment Further information is requested

Comments

@yurishkuro
Copy link
Member

I am just curious if there was some consideration behind the put name, and if so to document it here.

@SergeyKanzhelev
Copy link
Member

my understanding was always that set has a semantics of first and only write. put is used as "create or overwrite". But I never put much thoughts into it. So worth discussing.

Is it set in OT?

@bogdandrutu
Copy link
Member

I think this is very language specific. In Java we went with the "map" terminology of put, and it has the same semantics.

@yurishkuro
Copy link
Member Author

@SergeyKanzhelev yes, it's SetTag/setTag/set_tag in OT Go/Java/Python

@bogdandrutu are you saying the name would change in a different language? That will probably be rather confusing.

@SergeyKanzhelev SergeyKanzhelev added the Terminology alignment Further information is requested label Mar 28, 2019
@SergeyKanzhelev
Copy link
Member

I'm fine either way

@bogdandrutu
Copy link
Member

@yurishkuro If we want to have generic names I am fine with that, though some java devs will get confused about terminology.

One option is to use standard terminology for all these "map" like operations (we use this in tags in golang):

  • insert - If k already exists, doesn't update the value.
  • update - If k doesn't exists, doesn't insert the value.
  • upsert - It inserts the value if k doesn't exist already, it updates the value if k already exists.
  • delete - deletes the value associated with keys.

@yurishkuro
Copy link
Member Author

I prefer not over to complicate things. set is similar to classic setters, the upsert semantics of it seems universal. While put is very Java-centric, many other languages don't have an equivalent.

The one caveat from OT is that SetTag() had unspecified behavior if called more than once with the same key. This wasn't related to set vs put, more to the implementation and efficiency (e.g. Jaeger keeps KVs in array, rather than a map, so guaranteeing uniqueness would actually be extra work). There was never a particularly strong use case for the multi-map semantic.

@bogdandrutu
Copy link
Member

I agree that the implementation can keep everything in an array, but when exported to you dedup?

@yurishkuro
Copy link
Member Author

You can, but it's still extra CPU cycles and memory allocations that become necessary if the API semantics require key uniqueness.

@bogdandrutu
Copy link
Member

Then we should say that attributes is a List<Pair<Key, Value>>, so a better name is "addAttribute" probably.

@carlosalberto
Copy link
Contributor

I prefer not over to complicate things. set is similar to classic setters, the upsert semantics of it seems universal. While put is very Java-centric, many other languages don't have an equivalent.

I agree on this one - I think we can either go with addAttribute or setAttribute, even if it's not so Java centric.

@bogdandrutu
Copy link
Member

Decision: setAttributes, the behavior is a List of attributes and if the backend dedups then the ordering should be guaranteed.

@SergeyKanzhelev SergeyKanzhelev added the Agreed Issues that the group agreed on. label Mar 29, 2019
jkwatson pushed a commit that referenced this issue Aug 7, 2020
* Removed URLEncoder

* Fixed typo

* Added URLDecoding

* Included comment for string replacement

* Added unit tests for special characters in span names

* Resolved URL decoding issues

* Moved url decoding to parseQueryMap and updated the corresponding unit tests

* Added a README file for zPage quickstart

* Add images for README

* Updated README

* Add frontend images

* Add backend images

* Added our design doc

* Added details on package

* Reworded a few lines

* Moved DESIGN.md to a docs folder and changed gradle config to implementation

* Changed wording regarding HttpServer requirement

* Added zpages folder under docs, resolved broken image links

* Resolved comments for the design md file

* Made a few wording changes

* Wrote a benchmark test for TracezSpanBuckets (#23)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Updated README file (#25)

* Wrote benchmark tests for TracezDataAggregator (#24)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Added a set of benchmark tests for TracezDataAggregator

* Modified README formatting

* Changed benchmark test to negate dead code elimination

* Added Javadocs to the TracezDataAggregator benchmark tests

* Removed benchmark results from README and added a param to the TracezDataAggregator benchmark tests

* Update sdk_extensions/zpages/src/jmh/java/io/opentelemetry/sdk/extensions/zpages/TracezDataAggregatorBenchmark.java

Co-authored-by: Anuraag Agrawal <[email protected]>

* Added multiple param values for TracezDataAggregatorBenchmark

Co-authored-by: Terry Wang <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
jkwatson pushed a commit that referenced this issue Aug 12, 2020
* Removed URLEncoder

* Fixed typo

* Added URLDecoding

* Included comment for string replacement

* Added unit tests for special characters in span names

* Resolved URL decoding issues

* Moved url decoding to parseQueryMap and updated the corresponding unit tests

* Added a README file for zPage quickstart

* Add images for README

* Updated README

* Add frontend images

* Add backend images

* Added our design doc

* Added details on package

* Reworded a few lines

* Moved DESIGN.md to a docs folder and changed gradle config to implementation

* Changed wording regarding HttpServer requirement

* Added zpages folder under docs, resolved broken image links

* Resolved comments for the design md file

* Made a few wording changes

* Wrote a benchmark test for TracezSpanBuckets (#23)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Updated README file (#25)

* Wrote benchmark tests for TracezDataAggregator (#24)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Added a set of benchmark tests for TracezDataAggregator

* Modified README formatting

* Changed benchmark test to negate dead code elimination

* Added Javadocs to the TracezDataAggregator benchmark tests

* Removed benchmark results from README and added a param to the TracezDataAggregator benchmark tests

* Update sdk_extensions/zpages/src/jmh/java/io/opentelemetry/sdk/extensions/zpages/TracezDataAggregatorBenchmark.java

Co-authored-by: Anuraag Agrawal <[email protected]>

* Added multiple param values for TracezDataAggregatorBenchmark

* Changed TraceConfigz zPage form submit to use POST request

* Added requestMethod parameter to emitHtml, limited TraceConfig change on POST request only

* Removed duplicate parse function, added test for update on POST request only

* Added separate method for processing request

* Removed unnecessary error check in tests, used try resources for inputstream

Co-authored-by: williamhu99 <[email protected]>
Co-authored-by: William Hu <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agreed Issues that the group agreed on. Terminology alignment Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants