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

feat(message): Add Flag Support #264

Merged
merged 11 commits into from
Oct 24, 2024
Merged

feat(message): Add Flag Support #264

merged 11 commits into from
Oct 24, 2024

Conversation

jshlbrd
Copy link
Collaborator

@jshlbrd jshlbrd commented Oct 23, 2024

Description

  • Adds flag support to the message package
  • Refactors condition package, transform package, and cmd apps for message flags

Motivation and Context

Recent releases have been opinionated about how to process data and we should move toward a future where there is less "internal magic" and things happening without user's being aware. This is most evident in how messages are skipped during data transformation if the accessed value is missing:

value := msg.GetValue(tf.conf.Object.SourceKey)
if !value.Exists() {
	return []*message.Message{msg}, nil
}

This might be the best choice for most use cases, but unless you know that lines like this exist, you have no idea this is happening. (There are some other examples of this as well, but this PR is focused on addressing message skips.) Message flags are foundational to fixing this problem because they make it possible to embed configs directly in messages, like this:

msg := message.New().SetData([]byte(demoEvt))
if skipNull {
	msg = msg.SkipNullValues()
}

if skipMissing {
	msg = msg.SkipMissingValues()
}

if skipEmpty {
	msg = msg.SkipEmptyValues()
}

Using flags, packages (condition, transform, etc.) can check for attributes like this:

if msg.HasFlag(message.SkipNullValues) && val.IsNull() {
	doSomething(msg)
}

This isn't a new concept because this is what the ctrl attribute already does for messages (except it was not created within a larger system of flags). These are now equivalent:

if msg.IsControl() {
	return []*message.Message{msg}, nil
}
if msg.HasFlag(message.IsControl) {
	return []*message.Message{msg}, nil
}

This PR is a feature and refactor to avoid a breaking change (missing values still cause a message skip for most transform functions), but in the future we will likely have a configuration that looks like this so that it's configurable by users:

{
  messages: {
    skip_missing_values: true,
    skip_empty_values: true,
  },
  transforms: [
    ...
  ]
}

How Has This Been Tested?

  • All unit tests are passing.

Types of changes

  • 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 change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jshlbrd jshlbrd marked this pull request as ready for review October 24, 2024 14:16
@jshlbrd jshlbrd requested a review from a team as a code owner October 24, 2024 14:16
@@ -16,6 +16,15 @@ import (
const (
// metaKey is a prefix used to access the meta field in a Message.
metaKey = "meta "

// IsControl indicates that the message is a control message.
IsControl = iota
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: It might be more clear that these are part of the flag system if their names are: FlagIsControl, FlagSkipNullValues, etc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was avoiding stutter, like:

msg.HasFlag(message.FlagIsControl)

The Uber style guide has what looks like a good solution to this problem, so I added that.

@@ -44,6 +53,8 @@ type Message struct {
//
// Control messages trigger special behavior in transforms and conditions.
ctrl bool

flags []any
Copy link
Contributor

Choose a reason for hiding this comment

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

Is flags every a non int value from the iota above? If not, maybe this should have a stronger type than any.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@jshlbrd jshlbrd merged commit 1acc9a0 into main Oct 24, 2024
7 checks passed
@jshlbrd jshlbrd deleted the jshlbrd/feat/message-flags branch October 24, 2024 17:53
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