feat: Implement Enhanced Syslog for Dagger-Registry Redirect and Add Local Testing Guide #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gerhard
reviewed
Jul 26, 2023
gerhard
reviewed
Jul 26, 2023
gerhard
reviewed
Jul 26, 2023
added 3 commits
July 28, 2023 18:31
…Local Testing Guide This commit introduces several enhancements and new features: - Implemented a new Syslog in pkg/logger/logger.go with accompanying unit tests in logger_test.go. - Introduced the Syslog writer in pkg/syslogger/syslogger.go, along with a mock_syslogger for testing purposes. Unit tests for the syslogger are available in syslogger_test.go. - Updated main.go to integrate the new Syslog functionalities. - Made relevant updates to dependencies in go.mod and go.sum. - Added a comprehensive local testing guide (Local Testing Guide for Dagger-Registry Redirect.md) to facilitate local testing and verification of the Dagger-Registry Redirect. These improvements enhance the logging capabilities of the Dagger-Registry Redirect and streamline the process of local testing. Signed-off-by: grouville <[email protected]>
- Remove interface, as no mockups are required - Rename and simplify code - Modify syslogger unit tests (race conditions) - Add logger integration test (currently not testing http server graceful shutdown) Signed-off-by: grouville <[email protected]>
Still a race on the logger, I wonder what the best synchronization strategy is the best option: - a wg group at the handler level - a buffer at the logger level Signed-off-by: grouville <[email protected]>
Test deploying the app on fly, redirect to ngrok -> local vector instance Signed-off-by: grouville <[email protected]>
grouville
force-pushed
the
add-syslogger
branch
2 times, most recently
from
529141d
to
edf701a
Compare
E2E integration test in which we test our shutdown logic to see if we lose data when having a concurrent shutdown signal and incoming streams. In this test, we setup a local HTTP and TCP server (fake syslogger), send X messages to the HTTP server and right after that send the HTTP shutdown signal (faked with a channel). The HTTP server mimicks our main.go implementation: it wraps the handler with a waitGroup and waits for all processed logs to be written in the HTTP shutdown goroutine. Signed-off-by: grouville <[email protected]>
grouville
force-pushed
the
add-syslogger
branch
from
edf701a
to
52c3e71
Compare
- Update docs to follow new testing methodology - Implement app name retrieval from env Signed-off-by: grouville <[email protected]>
Update handler's logic, cleanup test, make app more generic with env variables Signed-off-by: grouville <[email protected]> Co-authored-by: Gerhard Lazu <[email protected]>
When closing the syslog server, I was not signaling the main goroutine that all messages had been processed. Also, easier implementation of httpserver Signed-off-by: grouville <[email protected]>
Rely on JSON formatting for easier processing on Vector's side Signed-off-by: grouville <[email protected]>
Implements the reconnection mechanism in logger's logic, when losing the TCP connection Signed-off-by: grouville <[email protected]>
grouville
force-pushed
the
add-syslogger
branch
from
e4cd3e5
to
0ca8c5a
Compare
Signed-off-by: grouville <[email protected]>
grouville
force-pushed
the
add-syslogger
branch
from
0ca8c5a
to
324a282
Compare
Guard against a segfault when the writer hasn't connected Signed-off-by: grouville <[email protected]>
Also sensible default values when running locally, nothing platform-specific. Signed-off-by: Gerhard Lazu <[email protected]>
So that this is set at deploy time & we have a single source of truth. Signed-off-by: Gerhard Lazu <[email protected]>
gerhard
force-pushed
the
add-syslogger
branch
2 times, most recently
from
27fdff0
to
da6d7de
Compare
Only deploy when the app source has changed (including the pipeline code). Remove logs target since it did not prove useful to have - we run this command locally when we need it. Share the same client when running multiple targets via All Signed-off-by: Gerhard Lazu <[email protected]>
We do not use the top-level go deps for GHA, everything runs in Dagger. Signed-off-by: Gerhard Lazu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://linear.app/dagger/issue/INFRA-36/mitigate-registrydaggerio-nats-spof
Problem
@gerhard and @jlongtine found out that when Fly.io NATS is affected, our ability to report metrics becomes affected too. See the original incident for more details.
Solution
This PR extends the app's logger to write to syslog as well as the current stdout.
As these are important changes, a markdown file
Local Testing Guide for Dagger-Registry Redirect.mdexplains how to test the app.Pre-requisites
Follow-up
fly.io(taking into consideration the new API v2 changes)~24hwhen we have the confidence that everything behaves as expected