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

[connector/countconnector] Handle non-string attributes #32225

Conversation

lkwronski
Copy link
Contributor

Description:

Adding support for non string attributes in the count connector. I started working based on #30314 (comment), but it doesn't work. When I try to add the type constaint:

type Number interface {
	int64 | float64
}

type AttributeConfig struct {
	Key               string  `mapstructure:"key"`
	DefaultValue      Number  `mapstructure:"default_value"`
}

This causes the following error:

 cannot use type Number outside a type constraint: interface contains type constraints

My idea was to introduce new fields DefaultIntValue and DefaultFloatValue to handle number types, let me know if this makes sense, if so, I will add documentation and finalize this PR.

Link to tracking Issue: Fixes #30314.

Testing: Added unit test

Documentation: TODO

Comment on lines 51 to 53
DefaultValue string `mapstructure:"default_value"`
DefaultIntValue int64 `mapstructure:"default_int_value"`
DefaultFloatValue float64 `mapstructure:"default_float_value"`
Copy link
Member

Choose a reason for hiding this comment

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

I think this design introduces unnecessary relationships between fields in the config. If we cannot use a type constraint, can we just make DefaultValue an any?

@lkwronski lkwronski force-pushed the lkwronski.issue-30314-count-connector branch from cfdb6ee to 4c59f7d Compare April 16, 2024 21:08
@@ -41,9 +41,23 @@ func (c *counter[K]) update(ctx context.Context, attrs pcommon.Map, tCtx K) erro
countAttrs := pcommon.NewMap()
for _, attr := range md.attrs {
if attrVal, ok := attrs.Get(attr.Key); ok {
countAttrs.PutStr(attr.Key, attrVal.Str())
} else if attr.DefaultValue != "" {
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep this check, or something equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

We need to check if there is a default value set. If not, won't the type switch below panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I added panic.

Copy link
Member

Choose a reason for hiding this comment

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

We do not rely on panics in the collector. There is no requirement to set a default value, so this is not even a problem. We should just skip the block, like we were doing previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope I understand now. I added a condition to skip if the default value is nil.

connector/countconnector/counter.go Show resolved Hide resolved
@lkwronski lkwronski force-pushed the lkwronski.issue-30314-count-connector branch 2 times, most recently from e7900e4 to 9a3064d Compare April 19, 2024 13:35
@lkwronski lkwronski force-pushed the lkwronski.issue-30314-count-connector branch from 9a3064d to b4495ed Compare April 21, 2024 14:47
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Agree with @djaglowski that the panic is out of place. Other than that, I like the solution given to the default value.

@lkwronski lkwronski force-pushed the lkwronski.issue-30314-count-connector branch from b4495ed to a9c2d20 Compare April 24, 2024 17:32
@lkwronski lkwronski force-pushed the lkwronski.issue-30314-count-connector branch from a9c2d20 to a9a6cd4 Compare April 24, 2024 17:32
@djaglowski djaglowski merged commit 780bce6 into open-telemetry:main Apr 25, 2024
171 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 25, 2024
@djaglowski
Copy link
Member

Thanks @lkwronski

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Count connector improperly handles non-string attributes
4 participants