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

[cmd/telemetrygen] Add Support for specifying Log Severity #30990

Merged

Conversation

Frapschen
Copy link
Contributor

resolve #26498

@Frapschen Frapschen requested a review from a team February 1, 2024 08:44
@github-actions github-actions bot added the cmd/telemetrygen telemetrygen command label Feb 1, 2024
@Frapschen Frapschen requested a review from mx-psi February 1, 2024 09:50
cmd/telemetrygen/internal/logs/logs.go Outdated Show resolved Hide resolved
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Feb 20, 2024
@Frapschen Frapschen removed the Stale label Feb 20, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

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

@github-actions github-actions bot added the Stale label Mar 6, 2024
@Frapschen Frapschen removed the Stale label Mar 18, 2024
@Frapschen Frapschen force-pushed the _Add_Support_for_specifying_Log_Severity branch from 73ee303 to 9fbb318 Compare March 18, 2024 04:01
@Frapschen Frapschen requested a review from mx-psi March 18, 2024 05:54
Frapschen and others added 2 commits March 19, 2024 19:15
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@Frapschen Frapschen requested a review from mx-psi March 26, 2024 02:26
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Apologies for the delay, I was out last week. This looks better, but I think we need to return errors when things are invalid, not use defaults

cmd/telemetrygen/internal/logs/config.go Outdated Show resolved Hide resolved
@@ -82,3 +86,42 @@ func Run(c *Config, exp exporter, logger *zap.Logger) error {
wg.Wait()
return nil
}

func parseSeverity(severityText string, severityNumber int32) (string, plog.SeverityNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using the default when invalid data is present, we should return an error.

For example, if severityNumber is outside of the valid range, we should return an error that says that the valid range is [1,24].

We may need to take care of the default case separately, but otherwise we shouldn't ignore what the user set, we should instead tell them that it is invalid.

@Frapschen Frapschen requested a review from mx-psi April 2, 2024 01:56
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me, LGTM! 🚀

@mx-psi mx-psi merged commit a7e26c8 into open-telemetry:main Apr 3, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 3, 2024
@Frapschen Frapschen deleted the _Add_Support_for_specifying_Log_Severity branch April 3, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/telemetrygen telemetrygen command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cmd/telemetrygen] Add Support for specifying Log Severity
3 participants