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

Remove extra labels types #1314

Merged
merged 9 commits into from
Feb 17, 2021
Merged

Conversation

dstdfx
Copy link
Contributor

@dstdfx dstdfx commented Nov 7, 2020

Fixes: #1300

@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #1314 (5c81520) into main (73194e4) will increase coverage by 0.1%.
The diff coverage is 64.7%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1314     +/-   ##
=======================================
+ Coverage   77.9%   78.0%   +0.1%     
=======================================
  Files        127     127             
  Lines       6673    6590     -83     
=======================================
- Hits        5203    5146     -57     
+ Misses      1223    1200     -23     
+ Partials     247     244      -3     
Impacted Files Coverage Δ
bridge/opentracing/internal/mock.go 74.3% <0.0%> (ø)
exporters/trace/jaeger/jaeger.go 93.0% <ø> (+8.3%) ⬆️
internal/rawhelpers.go 70.0% <ø> (-9.0%) ⬇️
label/key.go 100.0% <ø> (ø)
label/type_string.go 10.0% <0.0%> (-23.4%) ⬇️
exporters/otlp/internal/transform/attribute.go 91.6% <75.0%> (-8.4%) ⬇️
bridge/opentracing/bridge.go 47.8% <100.0%> (+8.8%) ⬆️
exporters/otlp/internal/otlptest/otlptest.go 74.5% <100.0%> (ø)
label/kv.go 82.1% <100.0%> (-1.6%) ⬇️
label/value.go 90.9% <100.0%> (+0.6%) ⬆️
... and 5 more

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Currently this change would remove support for unspecified types when being passed from code or default to encoding them as strings. It seems like a better user experience if we could provide more relevant transformations of the passed data (e.g. float32 -> float64, uint -> int64). Making sure these transformations are well documented in the functions that will perform them is going to be crucial.

bridge/opentracing/bridge.go Outdated Show resolved Hide resolved
@dstdfx dstdfx force-pushed the remove-extra-label-types branch 2 times, most recently from ca6cd9f to e4eba8b Compare November 14, 2020 20:35
@dstdfx dstdfx requested a review from MrAlias November 14, 2020 20:41
bridge/opentracing/bridge.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge.go Outdated Show resolved Hide resolved
@dstdfx dstdfx requested a review from Aneurysm9 January 7, 2021 11:55
Remove the following labels types: INT32, UINT32, UINT64
and FLOAT32.

Fix all extra labels types occurrences in the project.

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
@dstdfx
Copy link
Contributor Author

dstdfx commented Jan 9, 2021

@MrAlias seems like I can't beat codecov/patch, could we skip this check?

Base automatically changed from master to main January 29, 2021 19:39
@MrAlias MrAlias self-assigned this Feb 16, 2021
@MrAlias MrAlias merged commit 7de3b58 into open-telemetry:main Feb 17, 2021
@dstdfx dstdfx deleted the remove-extra-label-types branch February 17, 2021 07:11
ldelossa pushed a commit to ldelossa/opentelemetry-go that referenced this pull request Mar 5, 2021
* Remove extra labels types

Remove the following labels types: INT32, UINT32, UINT64
and FLOAT32.

Fix all extra labels types occurrences in the project.

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>

* Update CHANGELOG.md

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>

* Delete unused helpers

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>

* Convert passed values into remaining types

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>

* Clarify func description

* Fix uint64 convertion

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>

* Fix uint conversion

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>

* Update OTLP exporter label types

Co-authored-by: Tyler Yahn <codingalias@gmail.com>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove extra label value types
7 participants