Add new option for multiple attributes in topic attribute functions (metrics and logs) #696
Conversation
|
vigneshshanmugam
left a comment
vigneshshanmugam
left a comment
There was a problem hiding this comment.
Lets not do this as a breaking change for both MIS and MOTel, Maybe introduce new methods on the kafka config and deprecate the old ones.
|
@vigneshshanmugam It is no longer a breaking change now, both options are supported |
constanca-m
changed the title
| assert.NotNil(t, cfg.TopicAttributeFunc) | ||
| cfg.TopicAttributeFunc = nil |
There was a problem hiding this comment.
Since we wrap this function with multiple attributes, this is no longer valid, so I removed the check
|
Not sure why ci/run-tests is failing, running locally works fine: 
And 
|
kafka/manager.go
Outdated
| for _, response := range responses.Sorted() { | ||
| topic := strings.TrimPrefix(response.Topic, namespacePrefix) | ||
| logger := m.cfg.Logger.With(zap.String("topic", topic)) | ||
| if m.cfg.TopicLogFieldFunc != nil { |
There was a problem hiding this comment.
suggestion: we should remove usage of the deprecated func everywhere since it will be converted to the new func during creation
There was a problem hiding this comment.
I can't do that here because this is a field that is exportable and it would be a breaking change
There was a problem hiding this comment.
It should be fine, the constructor for the manager takes care of migrating the old func to the new one. Am I missing something ?
There was a problem hiding this comment.
First I must say I am not familiar at all with where this code is being used.
But looking at the MOTel controller (which was what brought me here), it uses this variable: https://github.com/elastic/hosted-otel-controller/blob/66ffdebb8a868d61501e93a8d5b0922eb849b509/internal/queuemanager/kafka.go#L50. I am afraid other services are using it as well
There was a problem hiding this comment.
that's ok, NewManager will take care of converting them to the new func during init. The internal code should then only use the new func
There was a problem hiding this comment.
You are totally right, I removed all usages of the deprecated functions, apart from the declaration in CommonConfig and merging them on finalize. I updated the test, it should all be working now.
Thank you so much for such careful review!! :)
4 participants
This would be a breaking change.
Relates to https://github.com/elastic/hosted-otel-controller/pull/173#discussion_r2299185735.