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

Too minimalistic approach for config documentation #838

Closed
damianprzygodzki opened this issue May 9, 2019 · 41 comments
Closed

Too minimalistic approach for config documentation #838

damianprzygodzki opened this issue May 9, 2019 · 41 comments
Assignees
Labels
enhancement pr needed closed bugs or features issues that require a PR. Because there is noone is willig to contribute it question wontfix

Comments

@damianprzygodzki
Copy link

I am totally confused with documentation that is constructed really poor.

I understand that we have topics to connect messages to processors, but there is no information about connecting this to transport queue. There is also no info about ability of configuration more clients (Clients or topics?). Naming in config is also confusing, like 'routerQueue' vs. 'defaultQueue'.

Wasted lot of time to research it on my own...

@Steveb-p
Copy link
Contributor

Steveb-p commented May 9, 2019

You're welcome to provide improvements to documentation.

As with many open-source projects, oftentimes documentation is satisfactory to the one writing the actual library, but not for end users - which is obviously due to the fact that author does have an understanding of what code does and "assumes" knowledge of it's internals.

At the very least please provide links to documentation pages that were confusing for you.

@damianprzygodzki
Copy link
Author

You're welcome to provide improvements to documentation.

It is totally hopeless idea to let person who struggle with bundle create documentation.

As with many open-source projects, oftentimes documentation is satisfactory to the one writing the actual library, but not for end users

That's why i created issue - to announce that there is a need.

At the very least please provide links to documentation pages that were confusing for you.

It is also funny because each of documentation available (official github repo or readthedocs service) has different and not up to date docs, so if you need URL, each one available is confusing.

Issue is about config documentation, config is documented in one chapter, how can i be more precise?

https://github.com/php-enqueue/enqueue-dev/blob/master/docs/bundle/config_reference.md

@Steveb-p
Copy link
Contributor

Steveb-p commented May 9, 2019

It is totally hopeless idea to let person who struggle with bundle create documentation.

Quite the contrary. You were looking for a particular option or configuration value and was unable to deduce what it stands for or where it can be found.
If you have it figured out - you said you spent time on it - then you're best suited to provide contribution since your knowledge is "fresh". If you did not, then ask a specific question.

As I said - even though I'm only "responsible" for Kafka part of library - I can imagine it might be difficult for maintainer to cater to everyone. Usually https://github.com/php-enqueue/enqueue-dev/blob/master/docs/bundle/quick_tour.md file is sufficient for most basic configurations, configuration reference that you have linked is just that - a config reference. Certainly it can be improved, but keep in mind we're spending our private time doing so - so pointing us in the right direction as specifically as possible is more than welcome.

If you want to discuss it further then drop me a message on gitter, so we don't pollute this issue with just our conversation. Just keep in mind I might not be online and I might not be able to answer your questions (since, again, I'm officially only looking after Kafka part of enqueue).

@damianprzygodzki
Copy link
Author

If you did not, then ask a specific question.

Thanks, i do.

client:
    traceable_producer:   true
    # What is prefix? Where it applies? Why it is defined as another parameter? Why it has to be always string and there is no ability to disable it? 
    prefix:               enqueue
    separator:            .
    # What is this parameter for?
    app_name:             app
    # What is router topic?
    router_topic:         default
    # Is it queue name for my topic?
    router_queue:         default
    router_processor:     null
    redelivered_delay_time: 0
    # Is it fallback for router_queue? Why do we have fallback? What is the case we need to define it here for?
    default_queue:        default

@sustmi
Copy link

sustmi commented May 14, 2019

I also currently struggle with the docs. If you don't mind, I will write my specific problems here. Or I can create separate issue from them if you want to keep this discussion focused on different topic.

1. Transport vs Client

Only during writing this comment I realized what is the difference between Client and Transport.
Surely, both terms are described in the quick tour: https://github.com/php-enqueue/enqueue-dev/blob/0.9.9/docs/quick_tour.md#transport
but I think those two look very similar and it would be nice to put them in a direct contrast in order to make these key concepts clear.

I suggest to start the Quick Tour guide with section called Transport vs Client. This section would first pinpoint that Transport is low-level while the Client is a higher-level abstraction for both production and consumption of messages.
Then it could have subsections that will be basically the current Transport and Client sections.

2. The documentation of message production using the Symfony bundle is not clear about the situation when there are multiple enqueue configurations

The Send event example shows that one should just get the ProducerInterface from dependency-injection container but it is not clear which one I will get and how can I get a specific one.
I realized that each configuration under the enqueue key is responsible for creating several services in the container. To name few of them:

  • enqueue.client.<configuration name>.context being instance of Interop\Queue\Context
  • enqueue.client.<configuration name>.producer implementing the ProducerInterface (being instance of Enqueue\Client\TraceableProducer)
  • enqueue.transport.<configuration name>.context again being instance of Interop\Queue\Context

There are also enqueue.client.default.producer and enqueue.transport.default.context which are aliases for the default services. (Based on the first in the YAML configuration?)

I suggest to use https://github.com/php-enqueue/enqueue-dev/blob/0.9.9/docs/bundle/config_reference.md as a place that explains what types of services are created by the individual enqueue configurations.
The https://github.com/php-enqueue/enqueue-dev/blob/0.9.9/docs/bundle/message_producer.md guide could then link to the config reference and mention that one can pick any of the producer sevices based on which configuration one wants to use.

I think that Doctrine Bundle's documentation does a decent job on explaining this: https://symfony.com/doc/master/bundles/DoctrineBundle/configuration.html#doctrine-dbal-configuration

The database_connection service always refers to the default connection, which is the first one defined or the one configured via the default_connection parameter.

Each connection is also accessible via the doctrine.dbal.[name]_connection service where [name] is the name of the connection.

3. Also, how are the individual configurations called? Are those "transports"?

The configuration reference calls the configurations by very general terms "key" and "prototype": https://github.com/php-enqueue/enqueue-dev/blame/0.9.9/docs/bundle/config_reference.md#L18
Should I call these "configuations of transports" or "clients" or "connections"?
Because when using simply "enqueue configurations", how do we know if we are talking about enqueue bundle configuration (as whole) or individual (connection|transport|client) configurations?

I suggest to pick a one distinct term for the single configuration and use it both in EnqueueBundle. Quick tour and Config reference sections.

4. It is unclear how to specify additional options for Kafka

The Kafka transport section says:

You can also you extended configuration to pass additional options, if you don't want to pass them via DSN string or need to pass specific options. Since rdkafka uses librdkafka (being basically a wrapper around it) most configuration options are identical to those found at https://github.com/edenhill/librdkafka/blob/master/CONFIGURATION.md.

OK, lets say that I want to set the request.timeout.ms option that libkafka accepts for topic configuration: https://github.com/edenhill/librdkafka/blob/v1.0.0/CONFIGURATION.md#topic-configuration-properties

Where should I put this configuration? The only place in the bundle's Config reference which looks like it accepts some additional options is driver_options in the client section.
But is it the right place? And why is it in client section and not transport section?
And what is driver anyway. Nowhere in the documentation is any explanation of term "driver", what does it mean in the context of this library and how it is related to transport and client.

Judging from the source code the only place that uses these driver_options is \Enqueue\Client\DriverFactory::createRabbitMqStompDriver() method.

I suggest to explain how exactly can one pass the librdkafka configuration options (and what type of options can be used: global or topic?) in the Kafka transport section. Also, it will be helpful to explain what "driver" means at least in the comment in Config reference that describes driver_options.

EDIT: I realized that the librdkafka configuration options go to global and topic section in the transport section.

What are your opinions on my suggestion? Do they make sense to you? Do you want to fix those issues by yourself @Steveb-p or should I create a pull request?

@Steveb-p
Copy link
Contributor

@sustmi thanks, I'll take a detailed look in the evening :)

@sustmi
Copy link

sustmi commented May 15, 2019

This may seem out-of-topic but since @damianprzygodzki already mentioned the poorly documented client options and I think this is related.

5. Why no messages arrived to my Kafka topic and what is router?

I am trying to send a message to a specific Kafka topic. I get no errors or warnings but no messages arrived to the topic yet.

My config.yml contains this:

enqueue:
    my_enqueue_subconfig:
        transport:
            dsn: "rdkafka://"
            global:
                metadata.broker.list: "%kafka_brokers%"
        client: ~

and I am sending the message like this:

$message = new \Enqueue\Client\Message('messageBody');

$this->producer->sendEvent(
    'my_kafka_topic_name',
    $message
);

where $this->producer is injected with service @enqueue.client.my_enqueue_subconfig.producer.

I tried to do some debugging and I found that this code gets executed: https://github.com/php-enqueue/enqueue-dev/blob/0.9.9/pkg/enqueue/Client/Driver/GenericDriver.php#L62
and $topic is instance of \Enqueue\RdKafka\RdKafkaTopic with $name set to enqueue.app.default instead of my_kafka_topic_name.

It looks like the topic name is set to [client.prefix].[client.app_name].[client.router_queue]. I don't get it. What is router? Is it some kind of abstraction? Why does RdKafkaTopic contain this topic name instead of the one I specified when sending the message?

@Steveb-p
Copy link
Contributor

Why does RdKafkaTopic contain this topic name instead of the one I specified when sending the message?

To clarify it on the spot: topic as enqueue understands it is NOT Kafka topic. Topic set in sendEvent method is only present in message properties. For all other purposes there is only one RdKafkaTopic object in use.

@Steveb-p Steveb-p self-assigned this May 15, 2019
@sustmi
Copy link

sustmi commented May 15, 2019

Maybe I got the whole Enqueue library wrong.
I thought about Enqueue as a Doctrine database library but for message queues.

But now I have a feeling that Enqueue is actually one level of abstraction higher. Instead of comparing it to a Database access library it could be compared to Entity-Attribute-Value model that stores all data in a single database table. Am I right?
I mean that it does not use the native topic functionality that Kafka provides but it implements its own topic system on top of any messaging system.
Instead of sending message to some arbitrary Kafka topic it sends all messages to a single enqueue.app.default Kafka topic and the actual ("virtual") topic name is part of the message itself.

Is this true or am I still misunderstanding this library?

@Steveb-p
Copy link
Contributor

Steveb-p commented May 15, 2019

@sustmi I can't say for sure, since I have only a rough idea how other transports work.

Technically speaking, there is not much preventing enqueue from using multiple Kafka topics for it's communication. It's only that at the moment when it was created it simply did not (I'd think about more like a proof of concept that Kafka can work as a transport).
EDIT: As a matter of fact, I'm pretty sure I've seen Symfony Messenger Adapter allowing control of which Kafka topic to write to, and it's loosely based on an adapter that uses enqueue.

There is definitely a lot of improvement that can be made. For example, current implementation of enqueue is unable of listening to multiple Kafka topics, even though the underlying library is capable of such consumption (see #832). Producers still need some love regarding poll method from KafkaProducer (see #749).
Functionaly, multiple Kafka topics will probably open the library (at least this particular transport) to truly use Commands from CQRS concept (possibly allowing replying to a commands, not only sending messages / events).

Working with multiple Kafka topics is constrained by unability of current Phprdkafka library to handle Kafka's Admin API (which was introduced in librdkafka in latest 1.0.0 release, which was sometime in february this year?). Otherwise enqueue will rely on topic defaults for those.
When ability to control Kafka topics arrive in phprdkafka it will allow controlling of topic retention and partitioning.

I'm pretty sure what you described (enqueue using already existing topic implementation in Kafka) is the way Makasim does intend to go with it eventually.

EDIT: I'm available most of the time on enqueue's gitter if you want to discuss it further and not pollute this issue thread.

@makasim
Copy link
Member

makasim commented May 21, 2019

The Send event example shows that one should just get the ProducerInterface from dependency-injection container but it is not clear which one I will get and how can I get a specific one.

Auto wire works for default transport\client (explicitly named default or the first configured).

@makasim
Copy link
Member

makasim commented May 21, 2019

It looks like the topic name is set to [client.prefix].[client.app_name].[client.router_queue]. I don't get it. What is router? Is it some kind of abstraction? Why does RdKafkaTopic contain this topic name instead of the one I specified when sending the message?

You mistake Enqueue Client Topic with Kafka ones. These are different concepts. The message does not get anywhere because there is no subscribers (processors) listening for that topic. Hence the message goes nowhere. In contrast, if there is no processor for the command an exception is thrown.

@makasim
Copy link
Member

makasim commented May 21, 2019

What is router?

Not all transports support pub\sub out of the box (file, redis, sqs to name a few). So we added a router concept to overcome that limitation.

@makasim
Copy link
Member

makasim commented May 21, 2019

I mean that it does not use the native topic functionality that Kafka provides but it implements its own topic system on top of any messaging system.

It depends on what you are using. If you use Enqueue Client then it is a high level abstraction and
of course it hides away some kafka parts. If you use rdkafka transport provide low level access to kafka features almost as is.

@makasim
Copy link
Member

makasim commented May 21, 2019

Doc improvements are very welcome. Please send us A PR

@makasim makasim closed this as completed May 21, 2019
@makasim makasim added pr needed closed bugs or features issues that require a PR. Because there is noone is willig to contribute it enhancement question labels May 21, 2019
@Steveb-p
Copy link
Contributor

@makasim can we keep issues open so they show up on the "issues" page? Unless they're really stale. I value the opinion of people here and would like to get as much feedback as possible and incrementally provide PRs :)

@makasim
Copy link
Member

makasim commented May 21, 2019

Then we should agree on what stale is.

A closed issue is still easy to find with GitHub search. Issues or bugs that stay for long without action are closed with the label "needs work". It would help to find such issues in the future.

@Steveb-p
Copy link
Contributor

Steveb-p commented May 21, 2019

They're still relatively easy to find, but closed state suggests that issue reported has been resolved.
Obviously we can and will discuss about it despite the issue state, but it can be misleading.

I just don't understand your reasoning for closing so many of them despite still expecting some kind of feedback in most of them (please don't take it as criticism, I just might not see your point / reasoning).

I'd suggest keeping them open unless they become stale, which in my opinion would be 30 days of no activity, other than maybe requests for feedback. I don't see the cost for keeping them open for a while.

Again, I understand that you're under heavy load as maintainer so please don't take it the wrong way. The choice is yours in the end :)

@makasim
Copy link
Member

makasim commented May 21, 2019

I am fine keeping them for 30 days. Let's stick to that rule.

From my experience, the work is either done right away or never.

@Steveb-p
Copy link
Contributor

I am fine keeping them for 30 days. Let's stick to that rule.

We can use https://github.com/probot/stale if you'd like to have it done automatically, so you won't have to deal with those.

Want me to prepare all the configuration and instructions for it for us?

@makasim
Copy link
Member

makasim commented May 21, 2019

That would be great

@makasim makasim reopened this May 21, 2019
@akrz
Copy link

akrz commented May 21, 2019

This is the most frustating PHP package I've ever used. The docs is in shambles, lots of 404 links. I understand that PRs are welcome but I don't think a lot of people are capable to improve them when a lot is entangled, obscured and, simply, a mess.

My recommendation to anyone that reads this and wants a simple rabbitMQ wrapper:

Cheers

@damianprzygodzki
Copy link
Author

Next thing i've found, totally useless - https://github.com/php-enqueue/enqueue-dev/blob/master/docs/transport/null.md.

What tools of enqueue it uses? Is it mock of transport or just NULL for everything? I read - it is doing nothing, but in functional testing chapter i see that you are tracing this message. If it is doing nothing than why we are tracing this message?

Maybe we should link it with https://github.com/php-enqueue/enqueue-dev/blob/master/docs/bundle/functional_testing.md#null-transport?

@cadavre
Copy link

cadavre commented May 22, 2019

The truth, from what I'm seeing, is that enqueue authors made their own concepts to implement abstraction over different queueing libraries/systems.

The problem is they rephrased some industry-standard keywords (like "topic") into their own understanding.

It's ok because you can invite rookie users to use queues, but it's enough rarely for a week or two. Eventually everyone ends up saying "ok, thats enough for me" or wanting to do some fine-tuning. Which enqueue doesn't allow. Let the example be RabbitMQ with AMQP direct exchange type – which cannot be accomplished.

Other problem is that there are way too many interfaces from different scopes (Enqueue itself, Psr, extension-dependent interfaces...). It's extremely hard even to debug the configuration as @damianprzygodzki mentioned.

As a final word – I think the biggest problem is lack of concepts documentation wrote over each queue implementation and "What you can and cannot do". If I would know that before I started using Enqueue – I wouldn't be using it in the first place.

@Steveb-p
Copy link
Contributor

@damianprzygodzki @cadavre Thanks for the input. I'm currently working on an issue at work which prevents me from starting to work on a PR for this, but I'll take it into account when I give it a go on friday.

Thanks for using enqueue, despite all it's flaws.

@Steveb-p
Copy link
Contributor

Documentation has been moved to github pages (https://php-enqueue.github.io/) and will be subsequently updated. It should be easier to navigate once #870 and #872 are merged.

@makasim
Copy link
Member

makasim commented May 28, 2019

@damianprzygodzki @cadavre please review concepts in #863 and let us know if that makes sense to you. You can comment on the PR. Thanks

@makasim makasim closed this as completed May 29, 2019
@makasim
Copy link
Member

makasim commented May 29, 2019

https://php-enqueue.github.io/concepts/

@Steveb-p
Copy link
Contributor

Steveb-p commented Jun 6, 2019

@sustmi Just wanted to let you know that despite the fact that this issue is closed I'm still keeping an eye on it and will - hopefully - incrementally adjust the docs with all the comments here.

@makasim
Copy link
Member

makasim commented Jun 6, 2019

@Steveb-p You could reopen it if you think it would be better to keep it that way.

@makasim
Copy link
Member

makasim commented Jun 6, 2019

The feedback on concepts is very welcome.

@Steveb-p
Copy link
Contributor

Steveb-p commented Jun 6, 2019

@Steveb-p You could reopen it if you think it would be better to keep it that way.

Since documentation issues pop up every now and then I'll open it and keep watch on it. If it becomes too cluttered I'll crawl it and move important bits to a separate issue, but for now it's manageable.

@Steveb-p
Copy link
Contributor

Steveb-p commented Jun 6, 2019

@makasim My own question is regarding Kafka topics and enqueue topics. It has been noted that those are two different things, but should enqueue go towards using multiple of Kafka's topics "inside" a single transport/client?

@makasim
Copy link
Member

makasim commented Jun 7, 2019

but should enqueue go towards using multiple of Kafka's topics "inside" a single transport/client?

To answer the question I need more details on what "Kafka multipe topics" means. Could you send some links where I can read about it ?

@sustmi
Copy link

sustmi commented Jun 10, 2019

@Steveb-p Thank you. The Key concepts section really helps to better understand the Enqueue library. Nice work!

I hope that I will find some time to improve the Symfony bundle part too in order to cover my points 2. and 4.

@Steveb-p
Copy link
Contributor

@Steveb-p Thank you. The Key concepts section really helps to better understand the Enqueue library. Nice work!

To give credit where credits due, I wasn't the author of the PR :) It was @sylfabre in #863

@sylfabre
Copy link
Contributor

@Steveb-p @sustmi thanks!

@stale
Copy link

stale bot commented Jul 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 10, 2019
@Steveb-p
Copy link
Contributor

Be silent, bot! I'll work on it. I promise! :)

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@agluh
Copy link

agluh commented Sep 1, 2020

Could anyone please give some examples (or at least some tips) of how each of this options in client config is used and what is difference between them? Didn't find any description here and in comments above.

prefix:               enqueue
separator:            .
app_name:             app
router_topic:         default
router_queue:         default
default_queue:        default
  1. For example, I want to send an event (or topic?)
$producer->sendEvent('a_topic', 'Hello there!');

What exchange will be used then? {prefix}{separator}{app_name}{separator}{???}

  1. If I want to send an RPC command as follow
$promise = $producer->sendCommand('a_processor_name', 'Hello there!', $needReply = true);

what exchange will be used? {prefix}{separator}{app_name}{separator}{???}

  1. Now I want to consume events (or topics?) in other app. What query will be used if processor looks like this
public static function getSubscribedTopics()
{
    return ['aTopic'];
}

Again, what pattern is used? {prefix}{separator}{app_name}{separator}{???}

So basically I don't understand difference between router_topic, router_queue and default_queue. Anyone help, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pr needed closed bugs or features issues that require a PR. Because there is noone is willig to contribute it question wontfix
Projects
None yet
Development

No branches or pull requests

8 participants