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

compute response.ok_pct correctly and add latency metrics for scenarios #4

Merged
merged 1 commit into from
Jan 26, 2018

Conversation

bagipriyank
Copy link

No description provided.

@@ -12,7 +12,7 @@
"license": "Apache-2.0",
"dependencies": {
"debug": "2.2.0",
"datadog-metrics": "^0.3.0"
"datadog-metrics": "^0.8.1"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least 0.8.1 is required to make sure

  1. count is used instead of counter as counter is deprecated in datadog
  2. count is not incremented for value 0

@@ -18,7 +18,7 @@ class DatadogPlugin
@ee.on 'done', @flushStats

getDatadogConfig: ->
flushIntervalSeconds: 0
# flushIntervalSeconds: 0
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

letting auto flush take over since that is how it was even with flushIntervalSeconds = 0 with v0.3.0 of datadog-metrics. datadog-metrics had a bug which was doing auto flush for flushIntervalSeconds = 0, but is now taken care of since 0.5.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identified a potential issue with "the last flush" - Artillery exits before it can be sent? For example, if the test is ~5 seconds, nothing gets sent to Datadog. This is true for both version 0.1.1 and the version in this PR. Needs further investigation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

'response.2xx': [0, datadog.increment]
'response.3xx': [0, datadog.increment]
'response.4xx': [0, datadog.increment]
'response.5xx': [0, datadog.increment]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all these metrics are actually counts, and hence we should treat them as datadog metric type count instead of gauge. it doesn't matter when running from one machine, but becomes important when used via serverless-artillery which uses multiple lambdas. these metrics then do not get combined if the metrics are treated as type gauge instead of type count.

percentage = (metrics['response.2xx'] + metrics['response.3xx']) * 100 \
/ metrics['scenarios.completed']
percentage = (metrics['response.2xx'][0] + metrics['response.3xx'][0]) \
* 100 / metrics['requests.completed'][0]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was making percentage > 100 as artillery is counting 200 and 300 as separate requests even when with in the same scenario.

@bagipriyank
Copy link
Author

Any update on this?

@anroots
Copy link
Contributor

anroots commented Jan 25, 2018

Hey - thank you for the PR and apologizes for not replying sooner (notifications -> /dev/null?). I will find some time to review & test the changes within a ~week.

@anroots anroots self-assigned this Jan 25, 2018
@anroots
Copy link
Contributor

anroots commented Jan 26, 2018

image

Lookin' good

@@ -1,3 +1,4 @@
node_modules/
src/*.js
test/*.js
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.idea/ is developer / machine specific, a global .gitignore is preferred.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for sharing that.

@@ -1,6 +1,6 @@
{
"name": "artillery-plugin-datadog",
"version": "0.1.1",
"version": "0.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will bump the minor version due to datadog-metrics update and the change from gauge to count.

@@ -18,7 +18,7 @@ class DatadogPlugin
@ee.on 'done', @flushStats

getDatadogConfig: ->
flushIntervalSeconds: 0
# flushIntervalSeconds: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identified a potential issue with "the last flush" - Artillery exits before it can be sent? For example, if the test is ~5 seconds, nothing gets sent to Datadog. This is true for both version 0.1.1 and the version in this PR. Needs further investigation.

@anroots anroots merged commit 27c186f into bigbank-as:master Jan 26, 2018
@anroots
Copy link
Contributor

anroots commented Jan 26, 2018

Thank you for the PR - merged and released 0.2.0.

Potential data flush issue described in #5, needs further work to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants