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

logger: adjust logger to receive logs blobs #1172

Conversation

iulianbarbu
Copy link
Contributor

Description of change

Enabled storing data for logs blobs.

How has this been tested? (if applicable)

Locally, through integration testing.

@iulianbarbu iulianbarbu changed the title Feature/eng 1071 adjust deployerruntimelogger to account for stdout runtime Adjust logger to receive logs blobs Aug 23, 2023
@iulianbarbu iulianbarbu changed the title Adjust logger to receive logs blobs logger: adjust logger to receive logs blobs Aug 23, 2023
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

I love all these red lines!

Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to remove these deps from shuttle-runtime too, which should be a nice compile time win for users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and the tests here need to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I actually removed the custom tracing layer injection. We'll have to follow up with a subscriber here if we want to structure the pre-user code logs or filter them. We'll probably want to set up some filtering within the business logic of the codegen, but still TBD.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add back a macro annotation for enabling a default tracing subscriber like the one we've been using thus far?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't for this PR. We will track though this discussion separately in a ticket.

logger/src/lib.rs Outdated Show resolved Hide resolved
logger/src/lib.rs Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

Have a few comments

logger/src/lib.rs Outdated Show resolved Hide resolved
logger/src/lib.rs Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

Looking good. Left small comments

codegen/src/lib.rs Outdated Show resolved Hide resolved
codegen/src/shuttle_main/mod.rs Outdated Show resolved Hide resolved
logger/Cargo.toml Outdated Show resolved Hide resolved
runtime/Cargo.toml Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
logger/tests/integration_tests.rs Outdated Show resolved Hide resolved
logger/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

LGTM 🥳

@iulianbarbu iulianbarbu merged commit 149e9b0 into shuttle-hq:feat/shuttle-logger-service Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants