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.

Sign up for GitHub

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

Jump to bottom

confgenerator : Add otel logging support for systemd_journald receiver. #1872

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

confgenerator : Add otel logging support for systemd_journald receiver. #1872

wants to merge 16 commits into from

Conversation

franciscovalentecastro
Copy link
Contributor

@franciscovalentecastro franciscovalentecastro commented Feb 3, 2025 •
edited
Loading

Description

Add otel logging support for systemd_journald receiver.

Related issue

b/394593541

How has this been tested?

Checklist:

  • Unit tests
    • Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-otel-logging-systemd branch from 6e369a7 to c731e31 Compare February 5, 2025 21:36
@franciscovalentecastro franciscovalentecastro changed the title [Draft] Add otel logging support for systemd_journald receiver. confgenerator : Add otel logging support for systemd_journald receiver. Feb 5, 2025
@franciscovalentecastro franciscovalentecastro marked this pull request as ready for review February 5, 2025 21:36
quentinmit
quentinmit reviewed Feb 7, 2025
View reviewed changes
integration_test/ops_agent_test/main_test.go
})
}

func testSystemdLog(t *testing.T, otel bool) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see this actually test the schema of the log entries, instead of just testing that one exists.

Probably that means we should send a particular canary message to journald and then check how it is represented in Cloud Logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a canary with different explicitly set priorities (e.g. INFO, ERROR) and verified the resulting message in Cloud Logging matches the expected schema. I'm thinking of ways this test could be improved (e.g. check the CODE_LINE, more priorities ... ).

@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-otel-logging-systemd branch 2 times, most recently from 32e04b0 to af35993 Compare February 14, 2025 16:33
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-otel-logging-systemd branch 2 times, most recently from eb0751e to f485480 Compare February 25, 2025 16:14
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-otel-logging-systemd branch from f485480 to 7416dee Compare February 25, 2025 18:41
quentinmit
quentinmit approved these changes Feb 25, 2025
View reviewed changes
confgenerator/logging_receivers.go
Fields: map[string]*ModifyField{
`severity`: {
CopyFrom: "jsonPayload.PRIORITY",
// This mapping sets SeverityText in otel logs data model which naming differs from Cloud Logging model.
Copy link
Member

Choose a reason for hiding this comment

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

We should be accepting the Cloud Logging values in severityText. Users can be writing this field directly, either with their own modify_fields or directly with JSON, and they can filter on those values. I think we should try to get this fixed in the exporter.

Note that the OTel spec allows (and encourages) arbitrary strings as severityText: https://opentelemetry.io/docs/specs/otel/logs/data-model/#severity-fields

For now I'd put this back to LogSeverity, change the test to only use severities where the name matches, and then file a bug to add support for the rest of the severities in the exporter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quentinmit quentinmit quentinmit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@franciscovalentecastro @quentinmit