Syslog drain app error messages in app log stream #633

corporatemax · 2024-11-25T12:35:07Z

Description

This change should resolve #579 .

I pulled the SyslogConnector.emitLoggregatorErrorLog function into the app_log_emitter.go file and moved its dependent variables accordingly and capsuled it with the AppLogEmitter struct.
Furthermore, I used the AppLogEmitter in the RetryWriter and the WriterFactory to provide error messages to the Application Developer who tries to configure a syslog drain but does not know why there are no logs showing up in the syslog drain target.
I did this also in the FilteredBindingFetcher as there missconfigurations leads to errors there.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Testing performed?

Unit tests
Integration tests
Acceptance tests

Checklist:

This PR is being made against the main branch, or relevant version branch
I have made corresponding changes to the documentation
I have added testing for my changes

If you have any questions, or want to get attention for a PR or issue please reach out on the #logging-and-metrics channel in the cloudfoundry slack

linux-foundation-easycla · 2024-11-25T12:35:12Z

The committers listed above are authorized under a signed CLA.

✅ login: corporatemax / name: Maximilian Stefanac (Corporate) (e270475, 25b8f3d, 56e1903, 1c03a02, 8e9af20, 9f789db)

chombium

@corporatemax Thanks for the PR and the time invested in it. Generally it looks good. I have only few minor comments and I would wait for the other approvers to tell what they think.

src/pkg/egress/syslog/loggregator_emitter.go

chombium · 2024-11-25T13:31:26Z

src/pkg/egress/syslog/loggregator_emitter.go

+
+// WriteLog writes a message in the application log stream using a LogClient.
+func (appLogEmitter *LoggregatorEmitter) WriteLog(appID string, message string) {
+	if appLogEmitter.logClient == nil || appID == "" {


I wonder if we should write a warning in the Syslog Agent's logs that the emitter is not set and the app dev won't know that something went wrong... On the other hand, I understand that this is only to safeguard the EmitLog call as everything is initialized properly in the syslog_agent.go cmd.

I feel like if we don't write a log message for the syslog agent, we are covering the misconfiguration... Of course, this might not be the write place as the log will be written on every WriteLog call.

@ctlong Any thoughts on this?

I am not sure if the location of this file is sufficient as I am also calling it in the filtered binding fetcher. Should I move this in its separate package? If yes where exactly?

Let's leave it as is for now until we think of something better.

@chombium it sounds like you're worried that we do not have any mechanisms to prevent future misconfiguration of the AppLogEmitter when it's initialized in syslog_agent.go, is that right?

I think we could adequately cover this case with a new test in syslog_agent_test.go. That would seem like a valuable addition to me.

src/pkg/egress/syslog/retry_writer_test.go

chombium · 2024-11-25T13:43:47Z

src/pkg/egress/syslog/syslog_connector.go

 	"golang.org/x/net/context"
+	"log"


Was this reformatting? If so please run golangci-lint run ./... in the /src folder

no this was goland - I ran the formatter on my side and commited the changes.

This looks very strange to me. Usually, the standard libraries are grouped at the top... can you please double-check the formatting here, thanks

src/pkg/ingress/bindings/filtered_binding_fetcher_test.go

chombium · 2024-11-25T13:59:41Z

src/pkg/egress/syslog/syslog_connector.go

@@ -138,7 +122,7 @@ func (w *SyslogConnector) Connect(ctx context.Context, b Binding) (egress.Writer
 		w.droppedMetric.Add(float64(missed))
 		drainDroppedMetric.Add(float64(missed))

-		w.emitLoggregatorErrorLog(b.AppId, fmt.Sprintf("%d messages lost for application %s in user provided syslog drain with url %s", missed, b.AppId, anonymousUrl.String()))
+		w.loggregatorEmitter.WriteLog(b.AppId, fmt.Sprintf("%d messages lost for application %s in user provided syslog drain with url %s", missed, b.AppId, anonymousUrl.String()))


This is a system/operator message and we should adjust the text as we have a concrete App for which some logs are lost. IMO x messages lost for syslog drain with url y is enough. On the other hand if the same syslog drain url is used for multiple apps the message would be clearer this way. Though, the app id will be sent anyhow as label/tag of the log. I'm not totally sure if we should change the message.

@ctlong any thought on this?

What is the final log message output to the user in this case? I think the log client, or some of the other logic in AppLogEmitter may prefix the message with some information. In any case, you're right, we should evaluate the full log envelope and see if anything should be tweaked to ensure that it is legible to an app developer.

chombium

@corporatemax Thanks for looking into the review and making adjustments. I'm happy with the how the things look like.

I wonder what @ctlong has to say about this PR.

chombium · 2024-12-02T15:02:56Z

@corporatemax Can you please check the unit tests? Do they fail because they run as GH Action or it's something else?

corporatemax · 2024-12-03T12:10:51Z

@chombium the test states

unexpected response from internal bindings endpoint. status code: 404

I assume this test is flaky regarding the opened ports.
Can you please rerun the unittests?

chombium · 2024-12-06T14:55:20Z

@corporatemax I was able to reproduce the error locally on Ubuntu Linux 20.04 LTS when running both all the tests and only the failing test suite.

All tests run:

go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --randomize-suites --fail-on-pending --keep-going --race --trace

and running the single suite:

go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --randomize-suites --fail-on-pending --keep-going --race --trace --focus "Syslog Agent App Suite"

At first I thought that we have some problem when all of the tests run together, but the problem is isolated, but it seems that the the problem is within the Syslog Agent App Suite.

I've also tried running single test like:

go run github.com/onsi/ginkgo/v2/ginkgo -r  --fail-on-pending --keep-going --race --trace  --focus "generates metrics" cmd/syslog-agent/app

and that run properly.

It's seems to me there are problems with how the tests are setup and run when running the whole Syslog Agent App Suite

I've rerun the tests as they run in the GitHub action and I could reproduce the problem. Therefore I will revoke my approval until all test are running properly

ctlong · 2024-12-10T00:31:40Z

src/pkg/egress/syslog/writer_factory.go

 	}
 }

-func (f WriterFactory) NewWriter(ub *URLBinding) (egress.WriteCloser, error) {
+func (f WriterFactory) NewWriter(ub *URLBinding, emitter AppLogEmitter) (egress.WriteCloser, error) {


What is the utility of providing WriterFactory with an AppLogEmitter and passing another AppLogEmitter to WriterFactory.NewWriter()? It seems to me like they are the same AppLogEmitter and so could be de-duped, but maybe I'm missing something.

chombium requested changes Nov 25, 2024

View reviewed changes

corporatemax requested a review from chombium November 25, 2024 16:38

corporatemax changed the title ~~Syslog drain app error messages~~ Syslog drain app error messages in app log stream Nov 26, 2024

corporatemax marked this pull request as ready for review November 27, 2024 10:53

corporatemax requested a review from a team as a code owner November 27, 2024 10:53

corporatemax added 6 commits December 2, 2024 09:29

add error messages from syslog drains to the app log stream

e270475

add error messages from filtered binding fetcher to the app log stream

56e1903

rename LoggregatorEmitter to AppLogEmitter

8e9af20

change format of NewFilteredBindingFetcher calls

25b8f3d

rename references to AppLogEmitter

1c03a02

move spyLogClient to testhelper package

9f789db

corporatemax force-pushed the syslog-drain-app-error-messages branch from bf6dd19 to 9f789db Compare December 2, 2024 08:39

chombium previously approved these changes Dec 2, 2024

View reviewed changes

ctlong reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syslog drain app error messages in app log stream #633

Syslog drain app error messages in app log stream #633

corporatemax commented Nov 25, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 25, 2024 •

edited

Loading

chombium left a comment

chombium Nov 25, 2024

corporatemax Nov 25, 2024

chombium Dec 2, 2024

ctlong Dec 10, 2024

chombium Nov 25, 2024

corporatemax Nov 25, 2024 •

edited

Loading

ctlong Dec 10, 2024

chombium Nov 25, 2024

ctlong Dec 10, 2024

chombium left a comment

chombium commented Dec 2, 2024

corporatemax commented Dec 3, 2024 •

edited

Loading

chombium commented Dec 6, 2024

ctlong Dec 10, 2024

Syslog drain app error messages in app log stream #633

Are you sure you want to change the base?

Syslog drain app error messages in app log stream #633

Conversation

corporatemax commented Nov 25, 2024 • edited Loading

Description

Type of change

Testing performed?

Checklist:

linux-foundation-easycla bot commented Nov 25, 2024 • edited Loading

chombium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corporatemax Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chombium left a comment

Choose a reason for hiding this comment

chombium commented Dec 2, 2024

corporatemax commented Dec 3, 2024 • edited Loading

chombium commented Dec 6, 2024

Choose a reason for hiding this comment

corporatemax commented Nov 25, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 25, 2024 •

edited

Loading

corporatemax Nov 25, 2024 •

edited

Loading

corporatemax commented Dec 3, 2024 •

edited

Loading