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

Support regex in topic protobuf mappings #405

Closed
upils opened this issue Jun 29, 2022 · 7 comments
Closed

Support regex in topic protobuf mappings #405

upils opened this issue Jun 29, 2022 · 7 comments
Labels
backend feature New feature or request

Comments

@upils
Copy link

upils commented Jun 29, 2022

For the protobuf topic mapping, we currently need to set the exact topic name.

We have a use case where our topic names are generated dynamically so we cannot give the full list in the configuration. But they all contains the same kind of message, so we would like to deserialize them all the same way.

We would like a way to give a regex in the topicName so all of our topics would match.

What do you think about this feature?

The current implementation is filling a map with the topicName as the key.

for _, mapping := range cfg.Mappings {

Then this map is used to check if a mapping exists for a given topic:

mapping, exists := s.mappingsByTopic[topicName]

We could consider the topicName as a regex, try to compile it when the config is loaded (and return an error if this is invalid) and keep using the given string as the key in the map.

Then after testing if the topic given in getMessageDescriptor() exists in the map, we would loop on the map to check if the topic match any regex. This would be a bit slower than the current implementation.

@weeco weeco added feature New feature or request backend labels Jun 29, 2022
@weeco
Copy link
Contributor

weeco commented Jun 29, 2022

Hey @upils ,
thanks for sharing your usecase. I think what you are suggesting makes sense to me. Do you want to work on this and submit a PR for this?

We should make sure that imports / nested types are supported. - regardless whether we use regexed strings or not. If I recall correctly we use protoreflect's proto registry to register all proto types in advance, but if your topics share the some proto schemas (with the same name) this should not be a problem I think.

@upils
Copy link
Author

upils commented Jun 29, 2022

Thanks for the reply.

Yes I might give it a try.

I am not sure to understand what you mean about the proto registry. It might be clearer if I dive in the code.

@weeco
Copy link
Contributor

weeco commented Jun 29, 2022

I was referring to this part

// Create registry and add types from file descriptors
registry := msgregistry.NewMessageRegistryWithDefaults()
for _, descriptor := range fileDescriptors {
registry.AddFile("", descriptor)
}
, but I think it's not relevant to your problem, because you are sharing the same protos across several topics and you only want to tell Console for what topics it should use the proto schemas.

@upils
Copy link
Author

upils commented Jun 29, 2022

OK. Yes, I understand it should not be an issue.

@smatvienko-tb
Copy link
Contributor

Hey, guys!
I need some regex in topic protobuf mappings!

I have a lot of protos for ThingsBoard and many topics created in flight with the service_id suffix.

A piece of my config looks weirdly long and does not cover deployments for any scale. And makes some pain in my eyes 😭.

I would be happy if you could put the final effort into finishing such a great feature!
image

Here is an example of a small part of ThingsBoard's mappings :

kafka:
  protobuf:
    enabled: true
    mappings:
      ####################     CORE     ######################
      - topicName: tb_core.0
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.1
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.2
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.3
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.4
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.5
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.6
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.7
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.8
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.9
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.10
        valueProtoType: transport.ToCoreMsg
      - topicName: tb_core.11
        valueProtoType: transport.ToCoreMsg
      ##########################################
      - topicName: tb_core.notifications.tb-core-0
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-1
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-2
        valueProtoType: transport.ToCoreNotificationMsg      
      - topicName: tb_core.notifications.tb-core-3
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-4
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-5
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-6
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-7
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-8
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-9
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-10
        valueProtoType: transport.ToCoreNotificationMsg
      - topicName: tb_core.notifications.tb-core-11
        valueProtoType: transport.ToCoreNotificationMsg
      #################### INTEGRATIONS ######################
      - topicName: tb_ie.api.requests
        valueProtoType: integration.IntegrationApiRequestMsg
      ##########################################
      - topicName: tb_ie.api.responses.tb-ie-main-0
        valueProtoType: integration.IntegrationApiResponseMsg
      - topicName: tb_ie.api.responses.tb-ie-main-1
        valueProtoType: integration.IntegrationApiResponseMsg
      - topicName: tb_ie.api.responses.tb-ie-main-2
        valueProtoType: integration.IntegrationApiResponseMsg
      - topicName: tb_ie.api.responses.tb-ie-other-0
        valueProtoType: integration.IntegrationApiResponseMsg
      - topicName: tb_ie.api.responses.tb-ie-other-1
        valueProtoType: integration.IntegrationApiResponseMsg
      - topicName: tb_ie.api.responses.tb-ie-other-2
        valueProtoType: integration.IntegrationApiResponseMsg
      ##########################################

@brycefisher
Copy link
Contributor

I'd love to get this feature merged into master and shipped in a release. Are you still planning to work on this @upils or did you want another contributor to take over from here? No worries either way!

@bojand
Copy link
Member

bojand commented Jun 19, 2024

This should be addressed now via #1291.

@bojand bojand closed this as completed Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend feature New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants