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

feature/adding event hubs output plugin #1755

Conversation

polatengin
Copy link

Allow using Azure Event Hubs as an output target, so you can use --output eventhubs to fork k6 outputs to Azure Event Hubs.

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this @polatengin! I left a few minor comments, and the testing situation should be improved before we consider merging this.

Also make sure to run go mod vendor / go mod tidy and commit the changes, otherwise you won't get a passing build in CI.

The k6 team is not familiar with Azure Event Hubs, but from what I could gather it exposes a Kafka-compliant endpoint. Is there a reason why the current Kafka output or the new Kafka extension couldn't be used instead of adding a custom collector for it?

It also appears to be based on AMQP, which is a much more generic and widely used protocol. From that perspective it would be preferable if we added AMQP support instead (say, as a new k6 extension), which would close #1073, and supported Event Hubs that way.

I'm interested to hear your thoughts on this, and also from @na-- @mstoykov.

stats/eventhubs/collector.go Outdated Show resolved Hide resolved
Comment on lines +38 to +39
// Collector sends result data to the Load Impact cloud service.
type Collector struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update all documentation comments for this collector.

stats/eventhubs/collector.go Outdated Show resolved Hide resolved
stats/eventhubs/collector_test.go Outdated Show resolved Hide resolved
Name: sample.Metric.Name,
Contains: sample.Metric.Contains.String(),
}
assert.Equal(t, expected, row)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear what you're testing here... SampleToRow() is a CSV collector function, and here you're comparing two structs for equality, so I don't see how it could fail. 😕

In this file I would expect to see tests of the Collector methods. If you can't start a mock server to test against, consider abstracting away the c.client calls, e.g. by passing some interface you can mock out in the tests.

stats/eventhubs/config.go Outdated Show resolved Hide resolved
stats/eventhubs/config.go Outdated Show resolved Hide resolved
stats/eventhubs/config.go Outdated Show resolved Hide resolved
stats/eventhubs/config_test.go Outdated Show resolved Hide resolved
stats/eventhubs/config_test.go Outdated Show resolved Hide resolved
@imiric imiric requested review from na-- and mstoykov December 8, 2020 15:40
@na--
Copy link
Member

na-- commented Dec 8, 2020

I don't have the time to look at this PR closely right now, sorry... I am also not very familiar with Azure Event Hubs or Application Insights (#1757). But, in general, if we can use a generic open protocol to output metrics to some service, there needs to be a good reason not to use it and instead to use a custom SDK for it (https://github.com/Azure/azure-event-hubs-go in this case, from what I can see). Moreover, that dependency itself seems to depend on a dead library... 😕

In any case, I also wanted to say that we plan to majorly refactor how the k6 outputs are handled, currently targeting k6 v0.31.0 (Feb 2021). We aim to fix a bunch of issues (#1606, #1075, #1423 among others) and to also enable the addition of output extensions with xk6. So it's very unlikely that we'll merge this PR or #1757 before k6 v0.31.0. And even then, I can't promise that these outputs would be merged straight into the k6 core, they might be better off as extensions for a while, to mature. We'd need to gauge the user interest against the maintenance burden of such changes.

@simskij
Copy link
Contributor

simskij commented Dec 9, 2020

I can assist here if it's any help, @na--. I'm quite familiar with Azure 👍

@polatengin
Copy link
Author

I can assist here if it's any help, @na--. I'm quite familiar with Azure 👍

Hi @simskij thanks for your support offer. It's greatly appreciated 👍

@polatengin
Copy link
Author

In any case, I also wanted to say that we plan to majorly refactor how the k6 outputs are handled, currently targeting k6 v0.31.0 (Feb 2021). We aim to fix a bunch of issues (#1606, #1075, #1423 among others) and to also enable the addition of output extensions with xk6. So it's very unlikely that we'll merge this PR or #1757 before k6 v0.31.0

Hey @na-- , is there an Early Access program for the new output system with xk6? I'd like to be in such program if you have one, also please count me in if you're going to do some design sessions and looking for community contributions.

@imiric
Copy link
Contributor

imiric commented Dec 15, 2020

@polatengin There's no "Early Access" program for xk6, but you're welcome to try creating an extension and publishing it on GitHub. Proper documentation is still pending, but you can take a look at this article, and use existing extensions as examples. It's really quite straightforward :)

If you need any help or have any suggestions feel free to post on the forum or Slack.

@na-- na-- removed their request for review December 2, 2021 14:42
@mstoykov
Copy link
Contributor

mstoykov commented Mar 31, 2022

Hi @polatengin , thanks for this PR 🙇.

k6 has had support for output extensions for some time now. As of today it also has a template repo for output extensions. This plus all the other extension outputs should be enough for people to extract this code in an extension and use it without it being in k6 core.

Given that I am closing this PR.

@mstoykov mstoykov closed this Mar 31, 2022
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.

6 participants