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

OpenTelemetry should convert level to string #10

Closed
leahneukirchen opened this issue Jun 24, 2024 · 9 comments
Closed

OpenTelemetry should convert level to string #10

leahneukirchen opened this issue Jun 24, 2024 · 9 comments
Assignees
Milestone

Comments

@leahneukirchen
Copy link

I'm running Telemere 1.0.0-beta14 with io.opentelemetry/opentelemetry-exporter-otlp 1.39.0, sending to Alloy 1.1.1 to Loki 3.0.

By default, the message "level" (a symbol in Clojure) ends up as a string like ":error" in Loki, which is recognized as "unknown" level. I think signal->attrs-map should pass on (name level) as "level", so "level" is "error". Then Loki colors the events properly.

@ptaoussanis
Copy link
Member

@leahneukirchen Hi Leah, thanks for pinging about this - I'm not familiar with Loki, so it's helpful to hear about things like this 🙏

Telemere's Open Telemetry handler currently does the following:

  1. It sets the severity level of the log record based on the level.
  2. It otherwise uses str on all keyword log record attribute values.

So in your case 1 should be correct, but I guess that Loki is also checking certain attribute values ("level" for example)?

My initial preference was to prefer ":foo/bar" to "foo/bar" for keyword attribute values since that at least preserves some info about the type, but this'd be easy to change.

Which option makes the most sense to you?:

  • A: Just change the "level" attribute value to be "foo/bar" instead of ":foo/bar"
  • B: Change all attribute values to be "foo/bar" instead of ":foo/bar" (so this'd also affect the "kind", "id", "uid" keys, etc.).

@leahneukirchen
Copy link
Author

Yes, Loki doesn't seem to use the severity level (but I'm converting with Alloy from OTLP to Loki, so it may get lost there. I'm trying direct ingestion next.)

I think B is reasonable actually, as it's unlikely people mix symbols and strings there, and expect it to be different?

@ptaoussanis
Copy link
Member

Yes, Loki doesn't seem to use the severity level (but I'm converting with Alloy from OTLP to Loki, so it may get lost there.

👍

I think B is reasonable actually, as it's unlikely people mix symbols and strings there, and expect it to be different?

Just to confirm- where exactly are you seeing symbols? I didn't quite understand this part in your original message. I believe Telemere's log record attribute names should all be strings; are you seeing something different?

@leahneukirchen
Copy link
Author

I switched to OTLP now, and while this sets severity_number = 5, Loki doesn't seem to parse this.

I meant values, not names. Currently, kind, data_handler_id, level, detected_level (perhaps added somewhere else) are prefixed by :.

@ptaoussanis
Copy link
Member

I meant values, not names. Currently, kind, data_handler_id, level, detected_level (perhaps added somewhere else) are prefixed by :.

Okay, but just to confirm- the values should also be strings (not symbols).

So you're seeing: "level" (string key) mapped to ":error" (string value).
What you want: "level" (string key) mapped to "error" (string value, without the ":").

In case you're not very familiar with Clojure, it has distinct string, symbol, and keyword types - so I'm just confirming that you're not somehow encountering symbols unexpectedly.

@leahneukirchen
Copy link
Author

Yes, it's (str symbol), not (name symbol).

@ptaoussanis ptaoussanis added this to the Next beta milestone Jun 24, 2024
@ptaoussanis ptaoussanis self-assigned this Jun 24, 2024
@leahneukirchen
Copy link
Author

I created confusion by saying symbol (that's what 20 years of Ruby do to you), I meant keywords of course. :)

@ptaoussanis
Copy link
Member

No worries, thanks for circling back to clarify 👍

@ptaoussanis
Copy link
Member

Fixed on upcoming master

ptaoussanis added a commit that referenced this issue Aug 5, 2024
Before: `:foo/bar` -> ":foo/bar"
After:  `:foo/bar` ->  "foo/bar"
ptaoussanis added a commit that referenced this issue Aug 7, 2024
Before: `:foo/bar` -> ":foo/bar"
After:  `:foo/bar` ->  "foo/bar"
ptaoussanis added a commit that referenced this issue Aug 18, 2024
On reflection the choice to drop ":" (and so type info) from ALL attribute
keywords seems excessive. We can instead restrict the change only to
the :level value.

Feedback welcome.
ptaoussanis added a commit that referenced this issue Aug 19, 2024
On reflection the choice to drop ":" (and so type info) from ALL attribute
keywords seems excessive. We can instead restrict the change only to
the :level value.

Feedback welcome.
ptaoussanis added a commit that referenced this issue Aug 19, 2024
On reflection the choice to drop ":" (and so type info) from ALL attribute
keywords seems excessive. We can instead restrict the change only to
the :level value.

Feedback welcome.
ptaoussanis added a commit that referenced this issue Aug 19, 2024
On reflection the choice to drop ":" (and so type info) from ALL attribute
keywords seems excessive. We can instead restrict the change only to
the :level value.

Feedback welcome.
ptaoussanis added a commit that referenced this issue Aug 19, 2024
On reflection the choice to drop ":" (and so type info) from ALL attribute
keywords seems excessive. We can instead restrict the change only to
the :level value.

Feedback welcome.
ptaoussanis added a commit that referenced this issue Aug 19, 2024
On reflection the choice to drop ":" (and so type info) from ALL attribute
keywords seems excessive. We can instead restrict the change only to
the :level value.

Feedback welcome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants