-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
[breaking] Add err chan to log producer and don't panic on error #1971
[breaking] Add err chan to log producer and don't panic on error #1971
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Before starting the review of this PR, I'd like to let you know that I'd like to plan a total refactor of the log producers feature: #525, so would like to know if we could work on this as a solid feature, instead of patching something that is not as usable as desired |
@mdelapenya gotcha. Regarding what you wanna do there, we get the logs from log producer and then our log component has different log targets: in-memory, file, loki. It buffers all logs and then flushes then, when you want to. Anyway, I will leave the PR open, because what you wanna do doesn't sound like something easy and fast to do :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tofel I left a few comments before merging this. Please let me know what you think
… instead of erroring on incorrect values
@mdelapenya is there anything left for me to do before we can merge it? I checked the failure in the pipeline, but it doesn't seem related to my changes at all:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I added a NIT comment regarding a test method name, but I will merge it as soon it's changed, without waiting for the CI
fix typo Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Hey @Tofel I think the PR is fine, although I see a consistent failure in the tests when Ryuk is disabled. Please see https://github.com/testcontainers/testcontainers-go/actions/runs/7299330173/job/19892388225?pr=1971 There are two containers leaked. I thought it was caused for something else, but I see this consistent error build after build. Could you take a look please? OTOH, I was planning to include this PR in the next release (today), but finding this makes me postpone it to the following one. Hope you understand. |
@mdelapenya sure, totally understand, I'd do the same. Will have a look and fix, but probably won't be able to do it this year :/ |
Thankfully the rest of this year is really short 😂 . Thanks for understanding, see you next year 🎄 |
@mdelapenya should be fixed now, I didn't call Happy new year 🥂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tofel This LGTM, thanks for your time here 👏👏👏
I added a comment regarding the docs for the changes, in two places: the comments for the methods and our docs website. But I'd be fine with including the docs website in a separate PR if you prefer it.
Thanks!
@mdelapenya done! by website did you mean the markdown file you mentioned or something yet different? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the docs, I think it's now clear how to use the new features. I added two comments regarding formatting in the docs. Once fixed, I think this is ready to be merged.
done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your work here!
Would you mind checking if the linked issues/PRs can be potentially closed?
Cheers!
* main: chore: move internal/testcontainersdocker package's files to internal/core (testcontainers#2083) GenericContainer: in case of error: return a reference to the failed container (testcontainers#2082) [breaking] Add err chan to log producer and don't panic on error (testcontainers#1971) chore: enrich HTTP headers to the Docker daemon with the project path (testcontainers#2080) fix: align codeql versions in GH workflow (testcontainers#2081) chore(deps): bump go.mongodb.org/mongo-driver in /modules/mongodb (testcontainers#2065) chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.11 to 3.23.12 (testcontainers#2068) fix(modules.gcloud): pass as ptr to allow request customization (testcontainers#1972) chore(deps): bump github.com/twmb/franz-go in /modules/redpanda (testcontainers#2072) chore(deps): bump k8s.io/api, k8s.io/apimachinery, k8s.io/client-go from 0.28.4 to 0.29.0 in /modules/k3s (testcontainers#2078) chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#2066) chore(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (testcontainers#2077) bump google.golang.org/api from 0.153.0 to 0.154.0, cloud.google.com/go/spanner from 1.53.1 to 1.54.0, bump google.golang.org/grpc from 1.59.0 to 1.60.1 in /modules/gcloud (testcontainers#2076) chore(deps): bump github.com/aws/aws-sdk-go-v2 from 1.23.5 to 1.24.0 (credentials from 1.16.9 to 1.16.13, service/s3 from 1.47.1 to 1.47.7) in /modules/localstack (testcontainers#2075) chore(deps): bump github/codeql-action from 2 to 3 (testcontainers#2056) chore(deps): bump test-summary/action from 2.1 to 2.2 (testcontainers#2058) chore(deps): bump actions/setup-go from 4 to 5 (testcontainers#2057)
@mdelapenya: do you have another release planned? |
What does this PR do?
As per conversation I changed the way starting of log producer works. Now it won't panic if there's an error and instead send it over an error channel. When that happens producer state is cleaned, so that it can be started again. Callers can listen to to the error channel and if there's an error decide how to handle.
Also, it's now required to pass context timeout value, when starting log producer. I've added some boundaries there, it has to be within
<5,60>
seconds. If it's too low the http connection to get the logs is too short and some logs might get missed randomly (happened, when I was setting it to 1s), if it's too long the connection might timeout.Now... I was thinking about not breaking the method signature and instead adding a new field to the
Container
struct that would store the timeout by default set to 5 seconds and then having a setter method to modify it. If you think that's better I could look into it.Last, but not least. Tests are very simple ones. In our downstream project I have more complex ones which actually focus on testing a detached goroutine that's listening to errors and restarting log producer if it's needed. If current approach is accepted I could play around with Mockery and add some tests here as well.
Why is it important?
We don't want unrecoverable panics if there's an error, when reading logs. It should be up to client to decide what should be done if there's an issue.
Related issues
#1278
#1911
How to test this PR
No straight-forward way. In our downstream project we are using only a limited set of methods from
Container
interface, so I narrowed it even further, put them in a separate interface and in tests created a mocked container with very stripped-down log-related logic that allowed me inject errors at various points. Without it, though... it's not trivial since you'd need to either disrupt/pause the network connection with Docker container so that reading logs fails due to timeout. Or use a bogus container that makes the connection timeout.