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

Include a more descriptive debugger for the collector and processor #1830

Merged
merged 5 commits into from Apr 16, 2024

Conversation

nathannaveen
Copy link
Contributor

@nathannaveen nathannaveen commented Apr 9, 2024

Description of the PR

This PR incorporates a child logging system that records a unique documentHash. Since the documentHash is being added to the child logger, the entire logging path will contain the documentHash, greatly enhancing the comprehensibility of the log entries.

This PR also includes tests for the child logger.

Log Outputs for the Processor

{"level":"info","ts":1712930572.1896873,"caller":"parser/parser.go:80","msg":"parsing document tree with root type: CycloneDX","documentHash":"sha256:d49be087aece91236ed2e464f5c768028b4106f5e9cd572d9b7f8e57e0a11218"}
{"level":"info","ts":1712930572.1907766,"caller":"helpers/bulk.go:38","msg":"assembling Package: 73","documentHash":"sha256:d49be087aece91236ed2e464f5c768028b4106f5e9cd572d9b7f8e57e0a11218"}
{"level":"info","ts":1712930572.1945672,"caller":"helpers/bulk.go:54","msg":"assembling Source: 0","documentHash":"sha256:d49be087aece91236ed2e464f5c768028b4106f5e9cd572d9b7f8e57e0a11218”}
...

Log Outputs for the Collector

{"level":"info","ts":1712931336.685455,"caller":"collector/collector.go:135","msg":"Successfully marshaled document.","documentHash":"sha256:6b9873ca3443dc0080208d3bfa61274026bfd5191f147a943eae442ac4c07b38"}
{"level":"info","ts":1712931336.687148,"caller":"collector/collector.go:142","msg":"Successfully wrote document to blob store.","documentHash":"sha256:6b9873ca3443dc0080208d3bfa61274026bfd5191f147a943eae442ac4c07b38"}
{"level":"info","ts":1712931336.6875935,"caller":"collector/collector.go:148","msg":"Successfully created an event.","documentHash":"sha256:6b9873ca3443dc0080208d3bfa61274026bfd5191f147a943eae442ac4c07b38"}
{"level":"info","ts":1712931336.6876295,"caller":"collector/collector.go:154","msg":"Successfully marshaled document key.","documentHash":"sha256:6b9873ca3443dc0080208d3bfa61274026bfd5191f147a943eae442ac4c07b38"}
{"level":"info","ts":1712931336.6876893,"caller":"collector/collector.go:159","msg":"Successfully published event.","documentHash":"sha256:6b9873ca3443dc0080208d3bfa61274026bfd5191f147a943eae442ac4c07b38"}
{"level":"info","ts":1712931336.6877022,"caller":"collector/collector.go:161","msg":"doc published: file:///testdata/syft-cyclonedx-docker.io-library-docker.latest.json","documentHash":"sha256:6b9873ca3443dc0080208d3bfa61274026bfd5191f147a943eae442ac4c07b38”}
...

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If OpenAPI spec is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen nathannaveen force-pushed the nathan/addLogging branch 2 times, most recently from bcd844f to 7f8f50e Compare April 9, 2024 20:01
@nathannaveen nathannaveen marked this pull request as draft April 9, 2024 20:47
@nathannaveen nathannaveen force-pushed the nathan/addLogging branch 4 times, most recently from e9e5f87 to 7aaff71 Compare April 12, 2024 14:32
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen nathannaveen force-pushed the nathan/addLogging branch 2 times, most recently from c1fdece to 9723786 Compare April 12, 2024 14:36
@nathannaveen nathannaveen marked this pull request as ready for review April 12, 2024 14:36
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@pxp928
Copy link
Collaborator

pxp928 commented Apr 15, 2024

@nathannaveen just need to fix up the e2e test! Thanks!

Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Thanks @nathannaveen

Copy link
Collaborator

@SantiagoTorres SantiagoTorres left a comment

Choose a reason for hiding this comment

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

LGTM with what I think this is doing, but I just want to double check:

  1. this is not adding a debugger per-se, just another logging key to the logging object
  2. There is some re-factoring so as to remove duplicate code between the colector and the certifier ?

This latter part is not exactly necessary but somewhat of a drive-by consequence of working on this prt of the code.

Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Thank you!

@pxp928
Copy link
Collaborator

pxp928 commented Apr 16, 2024

LGTM with what I think this is doing, but I just want to double check:

  1. this is not adding a debugger per-se, just another logging key to the logging object
  2. There is some re-factoring so as to remove duplicate code between the colector and the certifier ?

This latter part is not exactly necessary but somewhat of a drive-by consequence of working on this prt of the code.

  1. That is correct, the logger already can emit "debug" logs. This is just using that. This adds a new "child" object to the logger so that the logs can be grouped together based on the "document" being collected/ingested. This allows for better tracking of which document failed where.

  2. Yeah, the certifier is not calling the publish at the moment (it needs to) and proper logs were being collected via the "collector" implementation of publish. So I killed that to get ready for future PRs.

@kodiakhq kodiakhq bot merged commit 358205b into guacsec:main Apr 16, 2024
8 checks passed
arorasoham9 pushed a commit to arorasoham9/guac that referenced this pull request May 17, 2024
…uacsec#1830)

* Subscriber Child Logger Working

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>

* Added Tests and a Child Logger for Publish

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>

* Fixed tests

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>

* add missing child logger to collector

Signed-off-by: pxp928 <parth.psu@gmail.com>

* add missing child logger to certifier

Signed-off-by: pxp928 <parth.psu@gmail.com>

---------

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Co-authored-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: Soham Arora <arorasoham9@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants