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

Implement the Value and KeyValue types #4914

Closed
31 tasks done
Tracked by #3827
MrAlias opened this issue Feb 12, 2024 · 5 comments · Fixed by #4949
Closed
31 tasks done
Tracked by #3827

Implement the Value and KeyValue types #4914

MrAlias opened this issue Feb 12, 2024 · 5 comments · Fixed by #4949
Assignees
Labels
area:logs Part of OpenTelemetry logs

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Feb 12, 2024

reference: https://github.com/open-telemetry/opentelemetry-go/pull/4725/files#diff-6176e76e800e6ce0edb44f3d31adaca4602b7cc03cbe54f5ea233a7f2edd17a0

  • Value type
    • Kind method
    • AsAny method
    • AsString method
    • AsInt64 method
    • AsBool method
    • AsFloat64 method
    • AsBytes method
    • AsSlice method
    • AsMap method
    • Empty method
    • Equal method
    • StringValue function
    • IntValue function
    • Int64Value function
    • Float64Value function
    • BoolValue function
    • BytesValue function
    • SliceValue function
    • MapValue function
  • KeyValue type
    • Equal method
  • String function
  • Int64 function
  • Int function
  • Float64 function
  • Bool function
  • Bytes function
  • Slice function
  • Map function
  • Add tests to cover all added functionality
@MrAlias MrAlias added the area:logs Part of OpenTelemetry logs label Feb 12, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 16, 2024

The reference implementation includes panics in many of the Value methods:

Do we want to panic in these cases? This could cause runtime panics for users using OTel and it seems like it might be violating the OTel exception policy.

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 16, 2024

Instead of panicking we could log an error and return a zero value?

Maybe also return a default value in the case of something like func (*Value) String() string which could just use fmt.Sprint?

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 16, 2024

For reference, the existing attribute package does not panic: https://github.com/open-telemetry/opentelemetry-go/blob/main/attribute/value.go

@pellared
Copy link
Member

pellared commented Feb 19, 2024

Given the OTel Spec says

API methods MUST NOT throw unhandled exceptions when used incorrectly by end users.

and what the existing attribute package does

I support @MrAlias proposal:

Instead of panicking we could log an error and return a zero value

Note

It would be good describe it in DESIGN.md as e.g. slog panics in equivalent methods.

@pellared
Copy link
Member

pellared commented Feb 20, 2024

I updated the prototype (reference implementation): 147762f

EDIT: I also created #4948

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants