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

Adding objectTrace logger to output configurable fields to a new log file from PickUpPlace #530

Merged
merged 8 commits into from
Sep 8, 2023

Conversation

arp-0984
Copy link
Collaborator

@arp-0984 arp-0984 commented Aug 3, 2023

No description provided.

@cfkoehler cfkoehler self-requested a review August 8, 2023 22:24
@ldhardy ldhardy self-requested a review August 14, 2023 15:20
@cfkoehler cfkoehler self-requested a review September 1, 2023 09:02
Copy link
Collaborator

@cfkoehler cfkoehler left a comment

Choose a reason for hiding this comment

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

I am good with this once the one comment I left is resolved.
I imported this into our other project and messed with the overrides and such and all worked as I would expect

src/main/config/logback.xml Outdated Show resolved Hide resolved
@cfkoehler cfkoehler added the feature A new feature that does not exist today label Sep 1, 2023
@cfkoehler cfkoehler added this to the v8.0.0-M4 milestone Sep 1, 2023
@arp-0984
Copy link
Collaborator Author

arp-0984 commented Sep 5, 2023

The Super-Linter job keeps failing: "DOCKERFILE_HADOLINT_LINTER_RULES rules file (/action/lib/.automation/../../contrib/docker/.hadolint.yaml) doesn't exist. Terminating..."

This may be an issue with the job itself - I saw it was updated recently. I'm not sure why my 1-line change would have caused this issue if it passed before.

Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Would like to see the implementation better encapsulated so Places can integrate with less coupling.

src/main/java/emissary/util/ObjectTracing.java Outdated Show resolved Hide resolved
src/main/java/emissary/pickup/PickUpPlace.java Outdated Show resolved Hide resolved
src/main/resources/emissary/util/ObjectTracing.cfg Outdated Show resolved Hide resolved
src/main/java/emissary/pickup/PickUpPlace.java Outdated Show resolved Hide resolved
@drivenflywheel
Copy link
Collaborator

If we think the events we emit we may need to have environment-specific fields, we should consider using the Service Provider pattern that leverages the support we have in the emissary.spi package. The ObjectTracing class could leverage an environment-specific "service" that's responsible for constructing the event-specific/stage-specific entries. The SPILoader bootstrapping would allow us to select the specific service implementation to use at runtime, but the ObjectTracing class would internally just be coded against the service interface. If that's too much for this initial PR, maybe we can address it in a follow-up PR.

@arp-0984
Copy link
Collaborator Author

arp-0984 commented Sep 6, 2023

If we think the events we emit we may need to have environment-specific fields, we should consider using the Service Provider pattern that leverages the support we have in the emissary.spi package. The ObjectTracing class could leverage an environment-specific "service" that's responsible for constructing the event-specific/stage-specific entries. The SPILoader bootstrapping would allow us to select the specific service implementation to use at runtime, but the ObjectTracing class would internally just be coded against the service interface. If that's too much for this initial PR, maybe we can address it in a follow-up PR.

I'm investigating this, not sure if it is best to do in this PR or a follow-up PR.

@drivenflywheel
Copy link
Collaborator

I'm investigating this, not sure if it is best to do in this PR or a follow-up PR.
Follow-up PR is probably the right choice. I just wanted to capture the potential approach.

@jpdahlke jpdahlke merged commit 600525b into NationalSecurityAgency:master Sep 8, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature that does not exist today
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants