Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Count connector improperly handles non-string attributes #30314

Closed
j-kap-t opened this issue Jan 5, 2024 · 6 comments · Fixed by #32225
Closed

Count connector improperly handles non-string attributes #30314

j-kap-t opened this issue Jan 5, 2024 · 6 comments · Fixed by #32225
Labels
bug Something isn't working connector/count Stale

Comments

@j-kap-t
Copy link
Contributor

j-kap-t commented Jan 5, 2024

Component(s)

connector/count

What happened?

Description

When specifying attributes the connector will lose the client provided value if it is not a string, such as for http.status_code. The connector calls .Str() without checking the type, which results in setting the value to an empty string if the original value is not a string.

Steps to Reproduce

Count an attribute sent by clients as a non-string value:

connectors:
  count:
    spans:
      http.status.codes:
        attributes:
          - key: http.status_code

Expected Result

The connector emits metrics with the counts split by http.status_code

Actual Result

The connector emits metrics with http.status_code set to an empty string.

Collector version

v0.91.0

Environment information

No response

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

@j-kap-t j-kap-t added bug Something isn't working needs triage New item requiring triage labels Jan 5, 2024
Copy link
Contributor

github-actions bot commented Jan 5, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@djaglowski djaglowski removed the needs triage New item requiring triage label Jan 9, 2024
@djaglowski
Copy link
Member

Thanks for reporting. I agree we need to handle other types as well. Any interest in submitting a fix for this?

@j-kap-t
Copy link
Contributor Author

j-kap-t commented Jan 11, 2024

@djaglowski I would be happy to provide a fix and it seems mostly straightforward, however this one part that isn't immediately obvious. Is there a preferred way to handle the type of the default value: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/connector/countconnector/config.go#L51?

@djaglowski
Copy link
Member

That's a good point. Maybe we should define a type constraint which includes a fixed set of types which we would accept. It's not clear exactly which types we should support, but perhaps we could start with a somewhat limited list and go from there.

@cwegener
Copy link
Contributor

I just ran into this bug today as well, with exactly the same scenario (http status code).

For the type constraints, should we start with:

  • floats
  • ints
  • string
    ?

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Mar 19, 2024
djaglowski pushed a commit that referenced this issue Apr 25, 2024
**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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working connector/count Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants