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

Define case sensitivity of map<string, any> keys in Logs Data Model #3889

Closed
wants to merge 14 commits into from

Conversation

pellared
Copy link
Member

@pellared pellared commented Feb 20, 2024

Per #3853 (comment)

Changes

Add clarifications to map<string, any> taken from https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/common and make the description more normative.

The spec is stable, but I think that it is rather a clarification or/and straightforward standardization.

@pellared pellared changed the title Add clarifications to map<string, any> in Logs Data Model taken from standard attribute spec Define case sensitivity of map<string, any> keys in Logs Data Model Feb 20, 2024
@pellared pellared marked this pull request as ready for review February 20, 2024 19:11
@pellared pellared requested review from a team February 20, 2024 19:11
@tigrannajaryan
Copy link
Member

This was previously an undefined behavior, the PR defines the behavior now and I think such changes should be allowed for Stable documents.

@open-telemetry/specs-approvers any objections to this approach to stability?

specification/logs/data-model.md Show resolved Hide resolved
specification/logs/data-model.md Outdated Show resolved Hide resolved
@pellared pellared requested a review from arminru February 27, 2024 17:02
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

The spec is stable, but I think that it is rather a clarification

👍

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Let's keep this open for a while to allow time to object in case someone thinks this is a breaking change.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

I think this is fine clarification, but that this information is already implied. The common knowledge definition of a map doesn't support duplicate keys. The common knowledge definition of a string is case sensitive, unless specified otherwise.

🤷

@MrAlias MrAlias mentioned this pull request Mar 6, 2024
@pellared
Copy link
Member Author

pellared commented Mar 7, 2024

Side note: Actually some logging libraries allow duplicate keys (https://go.dev/play/p/fzIswL0Le7j). OTLP (and JSON) allows also passing such data.

EDIT: I will create a separate issue (or PR for it).

@carlosalberto
Copy link
Contributor

Side note: Actually some logging libraries allow duplicate keys

Still good to merge this PR, given this detail, for the March's release?

@pellared
Copy link
Member Author

pellared commented Mar 8, 2024

Side note: Actually some logging libraries allow duplicate keys

Still good to merge this PR, given this detail, for the March's release?

@carlosalberto It is good to merge (even if we decide to remove it later).

EDIT: I created #3931

@carlosalberto
Copy link
Contributor

@pellared Oops, lint is failing.

@pellared
Copy link
Member Author

@pellared Oops, lint is failing.

The failure is not caused by the changes from this PR.

@arminru arminru added spec:logs Related to the specification/logs directory area:data-model For issues related to data model labels Mar 12, 2024
keys in the map are unique (duplicate keys are not allowed). The representation
of the map is language-dependent.
Value of type `map<string, any>` is a map of string keys to `any` values.
Case sensitivity of keys MUST be preserved.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the wording here is right. "Case sensitivity MUST be preserved" logically means "if something is case sensitive, it should stay case sensitive; if something is case insensitive, it should stay case insensitive". Doesn't it? 🤔

Perhaps a better wording would be:

The keys in the map are case-sensitive and MUST be unique (duplicate keys are not allowed).

@pellared
Copy link
Member Author

Changing to draft per #3938.
Especially given this PR contains normative language.

@pellared pellared marked this pull request as draft March 13, 2024 13:27
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 21, 2024
@pellared pellared closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model For issues related to data model spec:logs Related to the specification/logs directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants