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

Added support for structured logging in csaf_aggregator #530

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

oxisto
Copy link
Contributor

@oxisto oxisto commented Apr 18, 2024

This PR adds structured logging for the aggregator service. Currently, only the text handler is used, but I can extend this to use the JSON handler as well. In this case, probably some code that is shared between the aggregator and the downloader would need to be moved to a common package.

I was also wondering, whether this repo is moving to Go 1.21 at the future, since slog was introduced in to the standard lib in 1.21. So currently, this still relies on the x/exp package.

Fixes #462

This PR adds structured logging for the aggregator service. Currently, only the text handler is used, but I can extend this to use the JSON handler as well. In this case, probably some code that is shared between the aggregator and the downloader would need to be moved to a common package.

I was also wondering, whether this repo is moving to Go 1.21 at the future, since `slog` was introduced in to the standard lib in 1.21. So currently, this still relies on the `x/exp` package.

Fixes gocsaf#462
@s-l-teichmann
Copy link
Contributor

Thank for your contribution. I will review the PR as soon as I've git some time.

I was also wondering, whether this repo is moving to Go 1.21 at the future, since slog was introduced in to the standard lib in 1.21. So currently, this still relies on the x/exp package.

We clearly can do this as we already where there. The 'x/exp' stuff was only introduced in PR #514 to help backwards compatibility.

@oxisto
Copy link
Contributor Author

oxisto commented Apr 18, 2024

Thank for your contribution. I will review the PR as soon as I've git some time.

Looking forward to the feedback!

I was also wondering, whether this repo is moving to Go 1.21 at the future, since slog was introduced in to the standard lib in 1.21. So currently, this still relies on the x/exp package.

We clearly can do this as we already where there. The 'x/exp' stuff was only introduced in PR #514 to help backwards compatibility.

Ah I see. Well it should be more or less a case of just replacing the exp package with the slog package, once you decide to revisit Go 1.21.

@bernhardreiter bernhardreiter changed the title Added support for structured logging in csaf_aggretator Added support for structured logging in csaf_aggregator Apr 19, 2024
@tschmidtb51
Copy link
Collaborator

@bernhardreiter
Copy link
Member

@oxisto are you okay with the new license Apache-2.0 for your contribution #535 ?

Can you also do the Go Version update has part of this PR?

@bernhardreiter bernhardreiter removed the request for review from s-l-teichmann April 23, 2024 12:38
@oxisto
Copy link
Contributor Author

oxisto commented Apr 23, 2024

@oxisto are you okay with the new license Apache-2.0 for your contribution #535 ?

Yes, that is ok for me.

Can you also do the Go Version update has part of this PR?

Sure, I can do that and already use slog instead of the x/exp one.

Copy link
Contributor

@bernhard-herzog bernhard-herzog left a comment

Choose a reason for hiding this comment

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

I tested this briefly and it seems to work. I didn't check all the changed log statements, though.

@bernhardreiter
Copy link
Member

@oxisto a side question about Github: Is it possible for your to run the "Integration Test" workflow (https://github.com/oxisto/csaf_distribution/blob/main/.github/workflows/itest.yml ) because obviously I cannot do this on your clone and as it is not part of our automatic test I have not figured out how to run it on a pull request manually).

@bernhardreiter bernhardreiter self-requested a review April 24, 2024 14:43
@bernhardreiter
Copy link
Member

@oxisto in one commit message you are asking a few questions (which ideally shouldn't be in a commit message):

but I can extend this to use the JSON handler as well.

It should be similar to how the downloader handels it.

In this case, probably some code that is shared between the aggregator
and the downloader would need to be moved to a common package.

Yes common code should go into a common package, we already have util.

Both can be done in different PRs.

Copy link
Member

@bernhardreiter bernhardreiter left a comment

Choose a reason for hiding this comment

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

LGTM and works.
A symbolic link is called symbol twice, I recommend to change this.

Removing the verbose command line option and adding setting for the log-level would be the next step, however it might also affect the command line, so it can be delayed.

@oxisto
Copy link
Contributor Author

oxisto commented Apr 24, 2024

@oxisto in one commit message you are asking a few questions (which ideally shouldn't be in a commit message):

but I can extend this to use the JSON handler as well.

It should be similar to how the downloader handels it.

In this case, probably some code that is shared between the aggregator
and the downloader would need to be moved to a common package.

Yes common code should go into a common package, we already have util.

Both can be done in different PRs.

Sure, once this is merged I can have a look at what can be extracted into a common package, so that other tools can potentially use this as well and that behaviour is consistent across cmd.

Apart from that I have changed the symbol to sym link.

@oxisto
Copy link
Contributor Author

oxisto commented Apr 24, 2024

@oxisto a side question about Github: Is it possible for your to run the "Integration Test" workflow (https://github.com/oxisto/csaf_distribution/blob/main/.github/workflows/itest.yml ) because obviously I cannot do this on your clone and as it is not part of our automatic test I have not figured out how to run it on a pull request manually).

https://github.com/oxisto/csaf_distribution/actions/runs/8821657205/job/24218013269

Copy link
Member

@bernhardreiter bernhardreiter left a comment

Choose a reason for hiding this comment

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

works and LGTM

Copy link
Contributor

@bernhard-herzog bernhard-herzog left a comment

Choose a reason for hiding this comment

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

Looks good to me now

@bernhard-herzog bernhard-herzog merged commit 617deb4 into gocsaf:main Apr 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider structured logging for aggregator, too.
5 participants