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

feat(attribute): add int32 support #6299

Conversation

cpeliciari
Copy link

  • Extend Value with Int32Value and Int32SliceValue.
  • Extend KeyValue with Int32 and Int32Slice.
  • Extend the Key type with Int32 and Int32Slice methods.

- Extend Value with Int32Value and Int32SliceValue.
- Extend KeyValue with Int32 and Int32Slice.
- Extend the Key type with Int32 and Int32Slice methods.
Copy link

linux-foundation-easycla bot commented Feb 11, 2025

CLA Signed


The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute

A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed 64 bit integer.

Int32 is not spec compliant.
You should convert your int32 to an int64 within your codebase.

@cpeliciari
Copy link
Author

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute

A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed 64 bit integer.

Int32 is not spec compliant. You should convert your int32 to an int64 within your codebase.

What I created is a way to convert int32 values to int64. If you look at the commit, you’ll see that I’m not implementing int32 within the attribute, but rather performing a conversion.

I noticed that there is one for Int, but not for Int32.

// Int creates a KeyValue instance with an INT64 Value.
//
// If creating both a key and value at the same time, use the provided
// convenience function instead -- Int(name, value).
func (k Key) Int(v int) KeyValue {
return KeyValue{
Key: k,
Value: IntValue(v),
}
}

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

int32 within the attribute, but rather performing a conversion.

I think that adding this would be violating the Principle of Least Astonishment.

Moreover conversion (for slices) is coming with an overhead. I think it is in Go philosophy that things that needs to be converted or have additional cost are required to be done explicitly. "Write boring Go code".

If the specification would extend the to support int32 then we would need to make a behavioral change of Int32 variants that are introduced in this PR.

@pellared pellared added the blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Feb 17, 2025
@pellared
Copy link
Member

pellared commented Feb 17, 2025

Closing per #6299 (review)

@open-telemetry/go-maintainers, feel free to reopen or discuss at the SIG meeting if you disagree with my decision.

@cpeliciari, I still want to thank you for your contribution. I deeply appreciate your efforts and time spend on this PR. I simply think that adding this can bring confusion to other users of the API. Notice that the changes you are proposing can also done as (local) helper functions.

@pellared pellared closed this Feb 17, 2025
@cpeliciari cpeliciari deleted the feat/attribute-support-int32 branch February 17, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants