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

Logs API and SDK: logs attributes are allowed to have AnyValue values #4315

Open
lmolkova opened this issue Nov 23, 2024 · 9 comments · May be fixed by #4342
Open

Logs API and SDK: logs attributes are allowed to have AnyValue values #4315

lmolkova opened this issue Nov 23, 2024 · 9 comments · May be fixed by #4342
Labels

Comments

@lmolkova
Copy link
Contributor

Spec requires log API to support any in attribute types https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes

The log attribute model MUST support any type,
a superset of standard Attribute,
to preserve the semantics of structured attributes emitted by the applications.

LogRecord uses the same Attributes type as other signals and does not support any attribute value

AttributeValue = Union[
str,
bool,
int,
float,
Sequence[str],
Sequence[bool],
Sequence[int],
Sequence[float],
]

The recommendation is to:

  1. Keep supporting standard Attributes - see Attributes "hell" opentelemetry-specification#4201 for the context
  2. Add log-specific attributes type that can take anyvalue and accept it as well

As long as p2 can be done incrementally, the lack of it should not block logs API/SDK stability.
It might, however block stability of logging handler bridge which should be able to create anyvalue attributes.

Part of open-telemetry/community#1751

@aabmass
Copy link
Member

aabmass commented Nov 27, 2024

The recommendation is to:

  1. Keep supporting standard Attributes - see Attributes "hell" opentelemetry-specification#4201 for the context
  2. Add log-specific attributes type that can take anyvalue and accept it as well

We already have AnyValue here

# This is the implementation of the "Any" type as specified by the specifications of OpenTelemetry data model for logs.
# For more details, refer to the OTel specification:
# https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-any
AnyValue = Union[
str,
bool,
int,
float,
bytes,
Sequence["AnyValue"],
Mapping[str, "AnyValue"],
None,
]

IIUC, we should just use that in place of Attributes in the Logs API/SDK?

@lmolkova
Copy link
Contributor Author

we can't change Attributes definition for spans and metrics - those don't support AnyValue on the API level.
For logs, we should have a way to add AnyValue attribute, but we should make sure it's easy to keep using the same Attributes type as for spans and metrics.

I'm not an expert in Python API design and there could be much nicer ways to achieve it, but I'd create a LogAttributes union (with AnyValue supported) and accepted Union[Attributes | LogAttributes] in LogRecord ctor.

@aabmass
Copy link
Member

aabmass commented Dec 2, 2024

👍 I'm just proposing to change it for the Logging API only and leave other signals with the existing Attributes type.

I'm not an expert in Python API design and there could be much nicer ways to achieve it, but I'd create a LogAttributes union (with AnyValue supported) and accepted Union[Attributes | LogAttributes] in LogRecord ctor.

Since Attributes is a subset of AnyValue, there shouldn't be any functional different vs using AnyValue directly. Is this more of a readability concern?

@lzchen
Copy link
Contributor

lzchen commented Dec 2, 2024

IIUC, we should just use that in place of Attributes in the Logs API/SDK?

I believe we would replace AttributeValue with AnyValue. Isn't that all that is needed?

@lmolkova
Copy link
Contributor Author

lmolkova commented Dec 2, 2024

I think it's also important to allow standard Attributes to be populated on logs - see open-telemetry/opentelemetry-specification#4201

I.e. I should be able to write

attributes = Attributes(...)
counter.add(1, attributes=attributes)
logger.emit(LogRecord(..., attributes=attributes)

if LogRecord takes LogAttributes only, users would need to do manual conversion between types (which is inefficient and increases complexity).

PS: please ignore my concern if pythonic implicit type conversions make it irrelevant. In any case, I suggest to write a test that uses standard attributes on a log record and make sure it works and does not raise type-checker errors

@aabmass
Copy link
Member

aabmass commented Dec 2, 2024

PS: please ignore my concern if pythonic implicit type conversions make it irrelevant.

Ah yes that's the case, Attributes just describes a structural type which is implicit implemented, not a named class.

attributes = {"foo": "bar"}
counter.add(1, attributes=attributes)
logger.emit(LogRecord(..., attributes=attributes)

should work fine

@lzchen
Copy link
Contributor

lzchen commented Dec 3, 2024

I believe we would replace AttributeValue with AnyValue. Isn't that all that is needed?

We should probably still do this no?

@xrmx xrmx linked a pull request Dec 9, 2024 that will close this issue
11 tasks
@xrmx
Copy link
Contributor

xrmx commented Dec 9, 2024

Did a PR to check this, it's in draft because I don't want this in the next release.

@xrmx
Copy link
Contributor

xrmx commented Dec 13, 2024

Removed draft from #4342

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

5 participants