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

Add event framework (Log Lossy Feed events) #89

Merged
merged 3 commits into from
May 5, 2021

Conversation

Qinusty
Copy link
Contributor

@Qinusty Qinusty commented Apr 16, 2021

This PR introduces an event framework for driving defined system events
through a sink interface. A logrus logging sink is implemented
initially for logging event locally but event could also be
dispatched to alternative metrics/logging mechanisms.

The initial event which is added for this is to log and
identify potential loss of data between polling intervals.
This lossy feed problem is detailed in issue #83 and whilst
this doesn't fix the problem, it can help to identify and monitor
occurrences of this issue within a long running production environment.

Lossy feed event checking and dispatch is added as a utility in the feeds package
and is implemented within the currently supported feeds which have the potential to be lossy
(crates, npm, pypi, rubygems).

@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch from 78af451 to 04ba4bc Compare April 16, 2021 16:04
@Qinusty Qinusty changed the title Add event framework (Log Lossy Feed events) WIP: Add event framework (Log Lossy Feed events) Apr 16, 2021
@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch from 04ba4bc to 1c704e7 Compare April 16, 2021 16:21
events/handler.go Outdated Show resolved Hide resolved
@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch from 1c704e7 to 4503ae5 Compare April 20, 2021 16:26
@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch 2 times, most recently from 99cdf0c to b6c14e8 Compare April 22, 2021 12:34
@Qinusty Qinusty changed the title WIP: Add event framework (Log Lossy Feed events) Add event framework (Log Lossy Feed events) Apr 22, 2021
// Checks whether an event should be dispatched under the configured
// filter options.
// Options are applied as follows:
// - disabled event types are always disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space

default:
return nil, fmt.Errorf(
"unknown sink type '%v' provided to events configuration",
ec.Sink,
Copy link
Contributor

@tom--pollard tom--pollard Apr 23, 2021

Choose a reason for hiding this comment

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

Could be nice to provide the available sink types here in the error message? However this does then open the question if we should be doing similar for all config options so that's potentially suited for a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be easy to do now but would add an additional burden to adding sinks which could easily be missed. I think that adding documentation for the currently supported sinks/publishers/feeds etc is something that should be done for supporting user configuration. #97 is open to track this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Until #97 is resolved it may be worth just adding a small snippet to the example config in the README as part of this PR, highlighting it optionally enabled (not blocking on this however).

@tom--pollard
Copy link
Contributor

tom--pollard commented Apr 23, 2021

As this is essentially opening up a new data type that can be put 'on the wire', (including the scope to have pubsub sinks in a similar fashion to the existing package publishers) does it make sense to have a defined expectation on what constitutes an 'event'?.

The event introduced here exercises all of the methods provided by the interface (component, type, message) when generating the logrus output. Could DispatchEvent() potentially pass a struct (validated with a schema for expected fields, format etc as with packages) constructed from the interface methods to the event handlers AddEvent() instead of the event itself?

@Qinusty
Copy link
Contributor Author

Qinusty commented Apr 27, 2021

As this is essentially opening up a new data type that can be put 'on the wire', (including the scope to have pubsub sinks in a similar fashion to the existing package publishers) does it make sense to have a defined expectation on what constitutes an 'event'?.

I think that the definition of an event can remain quite flexible, the exact reasoning behind a new event can be discussed during the implementation and review of specific event additions. As for how verbose this becomes, we can review this at a later date but the sinks can dispatch events in a filterable manner e.g. to specific type topics for a kafka sink (for example).

The event introduced here exercises all of the methods provided by the interface (component, type, message) when generating the logrus output. Could DispatchEvent() potentially pass a struct (validated with a schema for expected fields, format etc as with packages) constructed from the interface methods to the event handlers AddEvent() instead of the event itself?

Yeah I think this might need some tweaking to have an easily method of encoding data for transmission. We might want some form of standardised event payload which the individual Event implementations can populate. The current method of having the sink switch based on the event type isn't very extendable for many sinks and many events.

@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch 2 times, most recently from 91c385b to 693dff8 Compare April 28, 2021 10:10
@Qinusty
Copy link
Contributor Author

Qinusty commented Apr 28, 2021

Extending the event framework for event serialization for future sinks is being tracked as part of issue #96.

@Qinusty Qinusty requested a review from tom--pollard April 28, 2021 10:15
Copy link
Contributor

@tom--pollard tom--pollard left a comment

Choose a reason for hiding this comment

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

Other than my open 'nit' comment this all looks good to me, taking into account the followup issues that have been generated. Feel free to resolve the README suggestion comment regardless.

@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch from 693dff8 to cae2d39 Compare May 4, 2021 11:12
Qinusty added 2 commits May 4, 2021 12:35
This PR introduces an event framework for driving defined system events
through a sink interface. A logrus logging sink is implemented
initially for logging event locally but event could also be
dispatched to alternative metrics/logging mechanisms.

The initial event which is added for this is to log and
identify potential loss of data between polling intervals.
This lossy feed problem is detailed in issue ossf#83 and whilst
this doesn't fix the problem, it can help to identify and monitor
occurances of this issue within a long running production environment.
Lossy feed detection occurs on a feed by feed basis and is up to the
implementor to enable through the use of a utility struct `LossyFeedAlerter`.
Adds lossy feed checking for npm, pypi, crates and rubygems
feeds using the event framework.
@Qinusty Qinusty force-pushed the qinusty/log-lossy-feeds branch from cae2d39 to 3ddf962 Compare May 4, 2021 11:37
@Qinusty Qinusty merged commit 7a5988a into ossf:main May 5, 2021
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