Issue/#477 prometheus #526

flaviostutz · 2020-07-02T02:42:03Z

Background

This is a complete initial implementation of Prometheus Exporter support as discussed in #477

Checklist

Git commit messages conform to community standards.
Each Git commit represents meaningful milestones or atomic units of work.
Changed or added code is covered by appropriate tests.

flaviostutz · 2020-07-03T02:40:47Z

There are some unit tests failing randomly. I think this is due to some bugs detected by fuzzy tests on the encode csv part. Seems like encode/decode generated an additional "\r" in URL string. I am trying to fix some of them, but I think none is related to Prometheus commit itself. What should we do?

tsenart

Thanks for working on this! I'd like us to try to keep the Attacker separate from Prometheus stuff. The way we can do that is by hooking into the results channel returned by the Attack method — for each Result that we read, we update Prometheus metrics in

vegeta/attack.go

Line 203 in d9b795a

case r, ok := <-res:

README.md

tsenart · 2020-07-04T09:37:08Z

docker-compose.yml

+ ports:
+ - 8880:8880
+ environment: 
+ - TARGET_URL=https://www.uol.com.br


Can we change this default?

Sure. I am changing to www.yahoo.com, is this ok?

tsenart · 2020-07-04T09:37:57Z

docker-compose.yml

+
+ vegeta:
+ build: .
+ image: flaviostutz/vegeta


Would love to build and publish an official project Docker image. I see you opened a separate issue for that. What would it take to get that done?

The Dockerfile/docker-compose.yml created really solves #525. We would close #525 with current PR.

For an official Docker image, you should create a new Repository in Dockerhub with tsenart/vegeta name and we could configure an automated build there that automatically build master "branch" to "latest" and any tagged git to a tagged image in Dockerhub. I have not prepared this Dockerfile thinking about being an official image, because it doesn't support all vegeta properties. We could discuss that in a separate issue if you like.

Another option is we can use a random name in "image" field, because this is only a local name (not necessary published in Dockerhub). What do you think?

tsenart · 2020-07-04T09:38:18Z

docker-compose.yml

+ - REQUESTS_PER_SECOND=5
+
+ prometheus:
+ image: flaviostutz/prometheus


Same for this image.

This is an extension of the official prometheus image. In this image I added the option to pass scrape configurations using environment variables. In official prometheus image, they place configuration files in volumes, which is not a good pattern because it generates a "stateful" situation on operation (one has to create a configuration file and place in volumes, oposed to simply setting configurations in ENV parameters).

If you like we can remove this service from docker-compose.yml.

tsenart · 2020-07-04T09:39:32Z

lib/attack.go

+var requestSecondsHistogram = promauto.NewHistogramVec(prometheus.HistogramOpts{
+ Name: "request_seconds",
+ Help: "Request latency",
+ Buckets: []float64{0.1, 0.2, 0.5, 1.0, 2.0, 5.0, 10.0, 20, 50},


How have you chosen these buckets?

Based on my experience. Do you have any other references? Surely we can change.

lib/attack.go

tsenart · 2020-07-04T09:43:57Z

lib/attack.go

+ "status",
+})
+
+var requestBytesInCounter = promauto.NewCounterVec(prometheus.CounterOpts{


Because Prometheus can be used as a library, two Attackers could be instantiated in the same Go process, so global variables for these metrics shouldn't be used.

Sure, but Prometheus lib is thread safe. The two attackers should share the same counters, right? (maybe I missed something)

If summing the two (or more) attackers is not the case, we should differentiate the metrics by a new label...

Co-authored-by: Tomás Senart <[email protected]>

flaviostutz · 2020-07-04T19:51:45Z

There are some unit tests failing randomly. I think this is due to some bugs detected by fuzzy tests on the encode csv part. Seems like encode/decode generated an additional "\r" in URL string. I am trying to fix some of them, but I think none is related to Prometheus commit itself. What should we do?

By setting a fixed seed to random generators I think those random errors should stop happening so that the tests would be repeatable. What do you think about flyingmutant/rapid#14?

…viostutz/vegeta into issue/tsenart#477-prometheus

flaviostutz · 2020-07-04T20:31:58Z

Thanks for working on this! I'd like us to try to keep the Attacker separate from Prometheus stuff. The way we can do that is by hooking into the results channel returned by the Attack method — for each Result that we read, we update Prometheus metrics in

vegeta/attack.go

Line 203 in d9b795a

case r, ok := <-res:

I will look into that.

Placing Prometheus outside "Attacker" lib would be great, but we would lose Prometheus support when someone is using the lib. Prometheus would work only from the command line then. The way it was implemented the lib will have Prometheus Exporter too.

Placing Prometheus support to the lib should even help the guys from nitishm/vegeta-server#17.

What do you think it would be a better pattern?

tsenart · 2020-07-04T20:43:51Z

You can factor out the Prometheus metrics into a struct type that gets instantiated via a constructors and add a method Observe that takes in a Result. Then it’s easy to use, just create a sub package in lib called prometheus. Sent via Superhuman iOS ( https://sprh.mn/[email protected] )

…

On Sat, Jul 4 2020 at 10:32 PM, Flavio Stutz < ***@***.*** > wrote: > >> >> >> Thanks for working on this! I'd like us to try to keep the Attacker separate >> from Prometheus stuff. The way we can do that is by hooking into the results >> channel returned by the Attack method — for each Result that we read, we >> update Prometheus metrics in >> https://github.com/tsenart/vegeta/blob/d9b795aec8585a0fb435072f68d842d596c332de/attack.go#L203 >> >> >> > > > > I will look into that. > > Placing Prometheus outside "Attacker" lib would be great, but we would lose Prometheus support when someone is using the lib. Prometheus would work only from the command line then. The way it was implemented the lib will have Prometheus Exporter too. Placing Prometheus support to the lib should even help the guys from nitishm/vegeta-server#17 ( nitishm/vegeta-server#17 ). What do you think it would be a better pattern? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub ( #526 (comment) ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAAQPD77RYLOCWCB2HHBODTRZ6GUVANCNFSM4OONPOFQ ).

flaviostutz · 2020-07-07T21:38:54Z

You can factor out the Prometheus metrics into a struct type that gets instantiated via a constructors and add a method Observe that takes in a Result. Then it’s easy to use, just create a sub package in lib called prometheus. Sent via Superhuman iOS ( https://sprh.mn/[email protected] )
…
On Sat, Jul 4 2020 at 10:32 PM, Flavio Stutz < @.*** > wrote: > >> >> >> Thanks for working on this! I'd like us to try to keep the Attacker separate >> from Prometheus stuff. The way we can do that is by hooking into the results >> channel returned by the Attack method — for each Result that we read, we >> update Prometheus metrics in >>

vegeta/attack.go

Line 203 in d9b795a

case r, ok := <-res:

>> >> >> > > > > I will look into that. > > Placing Prometheus outside "Attacker" lib would be great, but we would lose Prometheus support when someone is using the lib. Prometheus would work only from the command line then. The way it was implemented the lib will have Prometheus Exporter too. Placing Prometheus support to the lib should even help the guys from nitishm/vegeta-server#17 ( nitishm/vegeta-server#17 ). What do you think it would be a better pattern? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub ( #526 (comment) ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAAQPD77RYLOCWCB2HHBODTRZ6GUVANCNFSM4OONPOFQ ).

Great! So I will separate concerns but keep Prometheus Exporter capabilities in lib too, right?
Please, check the other revision items from this PR too :)

Thanks.

…senart#477-prometheus

flaviostutz · 2020-07-24T20:32:29Z

Closing this PR to continue review in #534

flaviostutz added 2 commits July 1, 2020 19:42

Adding Prometheus Exporter to attack tsenart#477

6dc5052

adding sample grafana dashboards with Prometheus data

787c3ef

flaviostutz requested review from tsenart and xla as code owners July 2, 2020 02:42

fixing nil point during unit tests

5fb973f

tsenart requested changes Jul 4, 2020

View reviewed changes

flaviostutz and others added 3 commits July 4, 2020 16:29

avoid errors on fuzzy tests with "impossible" line breaks in URLs

76a6288

Update README.md

2e67e3d

Co-authored-by: Tomás Senart <[email protected]>

Update lib/attack.go

2e8b183

Co-authored-by: Tomás Senart <[email protected]>

This comment has been minimized.

Sign in to view

flaviostutz added 2 commits July 4, 2020 17:03

removing mostly impossible test situation that is causing random errors

0102648

Merge branch 'issue/tsenart#477-prometheus' of https://github.com/fla…

e9717d8

…viostutz/vegeta into issue/tsenart#477-prometheus

flaviostutz added 2 commits July 24, 2020 10:20

commiting latest changes

5b79488

Merge branch 'master' of http://github.com/tsenart/vegeta into issue/t…

b431b0c

…senart#477-prometheus

flaviostutz mentioned this pull request Jul 24, 2020

Implementing Prometheus Exporter and documentation #534

Closed

3 tasks

flaviostutz closed this Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/#477 prometheus #526

Issue/#477 prometheus #526

flaviostutz commented Jul 2, 2020

flaviostutz commented Jul 3, 2020

tsenart left a comment

tsenart Jul 4, 2020

flaviostutz Jul 4, 2020

tsenart Jul 4, 2020

flaviostutz Jul 4, 2020

flaviostutz Jul 4, 2020

tsenart Jul 4, 2020

flaviostutz Jul 4, 2020

tsenart Jul 4, 2020

flaviostutz Jul 4, 2020

tsenart Jul 4, 2020

flaviostutz Jul 4, 2020

flaviostutz Jul 4, 2020

flaviostutz commented Jul 4, 2020

This comment has been minimized.

flaviostutz commented Jul 4, 2020

tsenart commented Jul 4, 2020 via email

flaviostutz commented Jul 7, 2020

flaviostutz commented Jul 24, 2020

Issue/#477 prometheus #526

Issue/#477 prometheus #526

Conversation

flaviostutz commented Jul 2, 2020

Background

Checklist

flaviostutz commented Jul 3, 2020

tsenart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaviostutz commented Jul 4, 2020

This comment has been minimized.

flaviostutz commented Jul 4, 2020

tsenart commented Jul 4, 2020 via email

flaviostutz commented Jul 7, 2020

flaviostutz commented Jul 24, 2020