swarrot / SwarrotBundle

A symfony bundle for swarrot integration
MIT License
89 stars 59 forks source link

Command options are ignored when extra config is set #101

Closed notFloran closed 7 years ago

notFloran commented 7 years ago

I have defined a comsumer with the following extras:

            extras:
                max_messages: 100
                max_execution_time: 300

And I want to consume only one message so I execute the following command :

sf swarrot:consume:voip_valorize_call citadel_voip_valorize_calls --max-messages 1

But the option max-messagesis ignored :

[MaxMessages] Max messages have been reached (100)

I think the problem came from this line : https://github.com/swarrot/SwarrotBundle/blob/master/Command/SwarrotCommand.php#L120

The + ignore already defined config, we can use a array_mergeto fix that.

odolbeau commented 7 years ago

Hi. Could you please paste your whole configuration? When you want to add extras config, it should be put under the right processor. In your case, I think you're adding extra configuration for your consumer (which doesn't know how to deal with those options). You can take a at the configuration reference to have an example.

notFloran commented 7 years ago

Hi !

My config :

swarrot:
    provider: pecl
    default_connection: rabbitmq
    default_command: swarrot.command.base
    connections:
        rabbitmq:
            host: "%rabbitmq_host%"
            port: "%rabbitmq_port%"
            login: "%rabbitmq_user%"
            password: "%rabbitmq_password%"
            vhost: "%rabbitmq_vhost%"
    consumers:
        voip_valorize_call:
            processor: voip.processor.valorize_call
            middleware_stack: # order matter
                - configurator: swarrot.processor.signal_handler
                - configurator: swarrot.processor.max_messages
                - configurator: swarrot.processor.max_execution_time
                - configurator: swarrot.processor.exception_catcher
                - configurator: app.processor_configurator.sentry_exception_catcher
                - configurator: swarrot.processor.ack
            extras:
                max_messages: 100
                max_execution_time: 300

The "extras" config works: when I execute the swarrot command without options 100 messages are handled. But when I want to override this config with the --max-messages 1 option of the swarrot command, that doesn't works : 100 messages are handled, not 1

odolbeau commented 7 years ago

Your config should looks like this:

swarrot:
    provider: pecl
    default_connection: rabbitmq
    default_command: swarrot.command.base
    connections:
        rabbitmq:
            host: "%rabbitmq_host%"
            port: "%rabbitmq_port%"
            login: "%rabbitmq_user%"
            password: "%rabbitmq_password%"
            vhost: "%rabbitmq_vhost%"
    consumers:
        voip_valorize_call:
            processor: voip.processor.valorize_call
            middleware_stack: # order matter
                - configurator: swarrot.processor.signal_handler
                - configurator: swarrot.processor.max_messages
                  extras:
                      max_messages: 100
                - configurator: swarrot.processor.max_execution_time
                  extras:
                      max_execution_time: 300
                - configurator: swarrot.processor.exception_catcher
                - configurator: app.processor_configurator.sentry_exception_catcher
                - configurator: swarrot.processor.ack

Each extra config should be attached to the relevant configurator. Can you confirm it works with this config?

I'll try to figure out why those options partly work when they are put under the customer instead of the right configurator.

BTW, I see that you're using a sentry_exception_catcher. Is it possible to share your code? Maybe in a gist if you think it's not something which can be merged in swarrot?

notFloran commented 7 years ago

@odolbeau That works, thanks for your help !

For sentry, the code is here : https://gist.github.com/notFloran/c0e5f6290c498c5bc18ae188ecd389f8

Do you think the code is ok for a PR ?

odolbeau commented 7 years ago

Sorry, just found some time to take a look at your gist.

The code looks good to me. I never used Sentry mysel. Is the $data a custom format you choose or is it documented somewhere? Is there a limit a size for the data you sent?

Don't hesitate to open a PR for this Processor. :)

notFloran commented 7 years ago

Sorry, just found some time to take a look at your gist.

no problem

The code looks good to me. I never used Sentry mysel. Is the $data a custom format you choose or is it documented somewhere?

$data is documented here -> https://docs.sentry.io/clients/php/usage/#optional-attributes

tags are used to find events in sentry and extra are additional data for the context.

Is there a limit a size for the data you sent?

In the doc I find this : Extra contextual data is limited to 4096 characters.

I will do some tests at work next week.

odolbeau commented 7 years ago

Thanks. :)

notFloran commented 7 years ago

sentry-php cut $data when the lenght > 1024 -> https://github.com/getsentry/sentry-php/blob/master/lib/Raven/Serializer.php#L97

I will do a MR for the sentry processor.

odolbeau commented 7 years ago

Perfect. :) Don't hesitate to add all those useful links into your MR. :)