-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add minimal support for array value type #1523
Add minimal support for array value type #1523
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1523 +/- ##
==========================================
+ Coverage 91.97% 92.09% +0.12%
==========================================
Files 254 259 +5
Lines 17286 17516 +230
==========================================
+ Hits 15899 16132 +233
+ Misses 989 984 -5
- Partials 398 400 +2
Continue to review full report at Codecov.
|
1d88ecd
to
ff62b3c
Compare
@@ -36,13 +36,13 @@ func (ts TimestampUnixNano) String() string { | |||
type AttributeValueType int | |||
|
|||
const ( | |||
AttributeValueNULL = iota | |||
AttributeValueNULL AttributeValueType = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using "AttributeValueNull" as name? Maybe in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to avoid UPPERCASE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an exported name used in contrib. I am not sure we want to touch it. Certainly not in this PR, which only adds arrays. Perhaps in a future PR if we want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy to review a future PR that does this
ff62b3c
to
d3a22ef
Compare
@@ -36,13 +36,13 @@ func (ts TimestampUnixNano) String() string { | |||
type AttributeValueType int | |||
|
|||
const ( | |||
AttributeValueNULL = iota | |||
AttributeValueNULL AttributeValueType = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy to review a future PR that does this
go.sum
Outdated
@@ -10,6 +10,7 @@ cloud.google.com/go v0.50.0/go.mod h1:r9sluTvynVuxRIOHXQEHMFffphuXHOMZMycpNR5e6T | |||
cloud.google.com/go v0.51.0/go.mod h1:hWtGJ6gnXH+KgDv+V0zFGDvpi07n3z8ZNj3T1RW0Gcw= | |||
cloud.google.com/go v0.52.0/go.mod h1:pXajvRH/6o3+F9jDHZWQ5PbGhn+o8w9qiu/CffaVdO4= | |||
cloud.google.com/go v0.53.0/go.mod h1:fp/UouUEsRkN6ryDKNW/Upv/JBKnv6WDthjR6+vze6M= | |||
cloud.google.com/go v0.56.0 h1:WRz29PgAsVEyPSDHyk+0fpEkwEFyfhHn+JbksT6gIL4= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all these changes in go.sum?
Array value type was added to AnyValue Protobuf definition but corresponding support in the Collector was missing. This commit adds AnyValueArray internal data type which wraps an a slice of AnyValue pointers. AnyValueArray supports minimal set of functions for now. We will add more functions if needed in the future.
d3a22ef
to
6ccd8cc
Compare
Merging. Contrib-test failure is unrelated is already being fixed in open-telemetry/opentelemetry-collector-contrib#831 |
* Bump github.com/apache/thrift to v0.16.0 * Exclude go.opencensus.io v0.19.1
Array value type was added to AnyValue Protobuf definition but corresponding
support in the Collector was missing.
This commit adds AnyValueArray internal data type which wraps an a slice of
AnyValue pointers.
AnyValueArray supports minimal set of functions for now. We will add more
functions if needed in the future.