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

Collector logging #1783

Closed
wants to merge 2 commits into from
Closed

Conversation

c-kruse
Copy link
Contributor

@c-kruse c-kruse commented Nov 14, 2024

  • Disables http access logging by default
  • Adds flow-logging-profile flag to allow opt-in for the logging of vanflow records with some coarse control over the volume of log events though sampling.

@c-kruse c-kruse requested a review from ajssmith November 14, 2024 00:33
@c-kruse
Copy link
Contributor Author

c-kruse commented Nov 14, 2024

@ajssmith wanted to get your take on this approach before cleaning it up a bit. I'm still on the fence about the "flow-logging-profile" flag.

Background: In skupper v1 the volume of logs coming from a collector handling a lot of vanflow records could be a problem. In v2 I dropped flow logging to avoid the issue. For the most part I have not missed them - I have been leaning more on some of the new internal metrics the collector publishes, but there's been a time or two I wish I had them.

Starting out on the problem, I went with a fully user configurable per-record-type-regex-matching sampling but found that was too much to configure as a user. I've since moved to offering "profiles" with different sampling strategies set at somewhat arbitrary levels. Thoughts on this approach? Other suggestions?

EDIT Went forward with cleaning out some of the original cruft. Considering this fully ready for review!

@c-kruse c-kruse marked this pull request as ready for review November 18, 2024 19:10
@c-kruse c-kruse requested a review from ted-ross November 18, 2024 19:15

var (
// loggingProfileMinimal logs 1 vanflow event per second (with bursts up to 32)
// reduces Link Record noise to 1 every ~20s.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ted-ross thought you may find this interesting. I know you've mentioned link record noise before.

I felt I had to suppress logging for LinkRecord messages in particular due to the constant churn of octets/octet rates. When under load, I think Connector and Listener records were also observed to be particularly busy.

Implements several "profiles" to control the quantity of vanflow record
logging the collector does. Defaults to none.

Signed-off-by: Christian Kruse <[email protected]>
@ajssmith
Copy link
Member

Thanks for this. Agree that having a way to control the logging of flow-events is needed.

While there are other, it seems that he primary use case for generating flow logs it to satisfy audit requirements or to support debug activities related to communications across the van.

Having the ability to dynamically turn on/off flow logging would be sufficient.

If user needs the logs for audit purposes, the sampling feature would likely not be used. The question is whether sampling would be useful for debugging or simply turning logs on/off for a period of time would be ok.

It has been suggested by a number of users that long term, the granularity of flow logging should be at the routing key level (e.g. generate flow logs for service a but not for service b).

@c-kruse
Copy link
Contributor Author

c-kruse commented Dec 2, 2024

Closing for now! There's a few nuances I think we should probably tackle here.

@c-kruse c-kruse closed this Dec 2, 2024
@c-kruse c-kruse mentioned this pull request Dec 10, 2024
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.

2 participants