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

Use structured logs (log.InfoS(...)) #4193

Closed
2 tasks
antgamdia opened this issue Jan 31, 2022 · 9 comments · Fixed by #4949
Closed
2 tasks

Use structured logs (log.InfoS(...)) #4193

antgamdia opened this issue Jan 31, 2022 · 9 comments · Fixed by #4949
Assignees
Labels
component/project (To be removed) good first issue kind/enhancement An issue that reports an enhancement for an implemented feature

Comments

@antgamdia
Copy link
Contributor

antgamdia commented Jan 31, 2022

As per the discussion in #4180 (comment)
and #3848 (comment), it would be nice if we moved to structured logs (more info here).

Specifically, it means:

  • Use log.InfoS(...) in our log traces.
  • Add a --logging-format to our deployments to let users customize the format; for instance, using json-
@antgamdia antgamdia added component/project (To be removed) kind/enhancement An issue that reports an enhancement for an implemented feature priority/low good first issue labels Jan 31, 2022
@castelblanque
Copy link
Collaborator

This would be great!
WIth this in place, getting the structured logs indexed in e.g. Kibana would be a no-brainer 😄

@kristinjeanna
Copy link

Hi,

I'm interested in working on this. Couple of questions:

  1. Is the intent that all go source files in the repo will require this change (i.e., wherever klog is used, switch the logging to structured)? Are some sources out-of-scope for this change?
  2. Do you wish this change to follow the same approach as that documented at the k8s page Structured Logging migration instructions?

@absoludity
Copy link
Contributor

Hi,

I'm interested in working on this.

Excellent, thanks @kristinjeanna !

Couple of questions:

1. Is the intent that _all_ go source files in the repo will require this change (i.e., wherever klog is used, switch the logging to structured)?

Yes (some exceptions below). You may even find places where klog isn't yet used, switching is fine.

Are some sources out-of-scope for this change?

Some services are being deprecated, so could be skipped. The two I can think of are kubeops and asset-svc. If it's not much extra work, feel free to include them, but otherwise OK to skip.

If you can break up the PRs by service, that'd make it trivial to review (or if the changes are small, no problem to join them or do it all in one PR, just mean if it results in a large change - but don't think it will be huge).

2. Do you wish this change to follow the same approach as that documented at the k8s page [Structured Logging migration instructions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md)?

Most of the recommendations in there seem sensible (ie. why not clean up the message formatting while there), so yes, please feel free to follow those recommendations.

Thanks again!

@akankshakumari393
Copy link
Contributor

Hi @antgamdia Is someone still working on it. Or I can take a stab on it.

@antgamdia
Copy link
Contributor Author

No, unfortunately, we haven't had the bandwidth to work on this. It'd be great if you can! Thanks!

@akankshakumari393
Copy link
Contributor

Cool thanks @antgamdia 👍

@akankshakumari393
Copy link
Contributor

Some services are being deprecated, so could be skipped. The two I can think of are kubeops and asset-svc. If it's not much extra work, feel free to include them, but otherwise OK to skip.

If you can break up the PRs by service, that'd make it trivial to review (or if the changes are small, no problem to join them or do it all in one PR, just mean if it results in a large change - but don't think it will be huge).

Hey @antgamdia How do we recognize which services are being deprecated? Also How do we figure out which service is in which package in the code?

@antgamdia
Copy link
Contributor Author

Hi @akankshakumari393 ,

main services are under /cmd, there you'll find:

  1. apprepository-controller --> s
  2. asset-syncer
  3. kubeapps-apis
  4. kubeops
  5. pinniped-proxy --> this one is written in Rust, so the approach should be slightly different; you can skip it.

We have successfully removed asset-svc already (#4880) and the next one is kubeops (it is currently used for AppRepos, although we are working on changing it (#4764) and Operators (#4920). So you can safely ignore this component as well, I'd say.

I'd recommend starting with kubeapps-apis, which is the most used service.

Besides, there are some common packages under /pkg. You can see the dependencies in this comment: #4847 (comment)

Anyway, happy to assist in the Slack channel if you want!

@akankshakumari393
Copy link
Contributor

Hi @antgamdia, I have raised a PR that adds changes in kubeapp-apis service. As there were too many files, breaking the PR into services would be better. Let me know if you have any queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/project (To be removed) good first issue kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants