[receiver/apache] send port as a resource attribute #16053
Conversation
receiver/apachereceiver/config.go
Outdated
| if u.Scheme == "https" { | ||
| cfg.port = httpsDefaultPort | ||
| } else if u.Scheme == "http" { | ||
| cfg.port = httpDefaultPort | ||
| } |
There was a problem hiding this comment.
The Validate function should not change the configuration.
I suggest moving the port field to the scraper and setting it in the factory.
There was a problem hiding this comment.
I understand the reason, but serverName is set in this function anyway - shouldn't it also be changed?
There was a problem hiding this comment.
Yeah, really it should. I think it's an inconsequential change but is better for the sake of consistency, so it's fine to do in this PR in my opinion.
There was a problem hiding this comment.
I think we have caught a bug here.
After the fix, the integration tests stopped passing - the expected value of serverName was "", while the receiver returned "localhost". I checked immediately - and it happens that Validate is called only inside unit tests - so outside the tests, serverName wasn't actually set and was always an empty string.
I fixed this and pushed the changes on this branch.
aboguszewski-sumo
force-pushed
the
apache-port-attribute
branch
from
9f84322 to
748aadc
Compare
|
I found an easy way to check the port in the tests anyway, so I modified it. |
…6053) * feat: add sending port as a resource attribute
…6053) * feat: add sending port as a resource attribute
Description:
Metrics sent by
apachereceivernow useapache.server.portresource attribute. This feature is hidden behind a feature gate.Link to tracking Issue: #14791
Testing:
Unit tests for the config and scraper.
Documentation:
mdatagen