WIP xk6 TimescaleDB output

[fridgepoet]

A PR in order to get any early feedback on the code itself as I do not know if I used some parts correctly like output.SampleBuffer and the periodic flushing.
Work was based on grafana/k6#1233 and the influxdb output implementation.

Locally, the build seems to be working (config from env variables only at the moment).
I'm able to see the samples table populated in my local timescale db, but I have not yet checked thresholds, push interval, concurrent writes...

To do:

• Add docker-compose
• Add README
• Add configurable log level

[mstoykov]

Hi @shirleyleu, thanks for the PR 🎉 .

I somewhat commented on some things that you mention in your TODO list that I forgot about while going through the PR, so just take them as suggestions on how it can be done ;)

Please ask if something isn't clear or you need more information, even within the code that we haven't commented on or just in general.

I in general am somewhat sceptical of how the thresholds are saved in timescale but that is outside the scope of this PR, so I guess we will discuss this after that.

[mstoykov]

Any reason for this instead of u.String() as in the original PR?

[fridgepoet]

Yes, so what I understood from our previous conversation is that the config argument should be able to be passed like this -out=timescaledb=postgres://user:password@localhost:5433/mydbname?push_interval=2s

u.String() would return postgres://user:password@localhost:5433/mydbname?push_interval=2s but this will return an error when parsed by pgxpool.ParseConfig because of the invalid query.

I decided to isolate and construct pgUrl to be passed to ParseConfig, though this might be a little misleading and users won't be able to connect with valid query parameters.... another way could be to reject setting push_interval by config argument here and only take it from json/env variable.

What are your thoughts?

[mstoykov]

I would've done all tests as one big table driven test. As in something like in the influxdb output

[fridgepoet]

Right, I thought this might raise some eyebrows. I do know what table tests are and consciously made a decision not to use them in this situation.
That said, the fact that I anticipated I would receive some criticism about it means that the reasons I have are not widely adopted. However, I do think that they’re valid.

I’m happy to discuss (debate 🙂 ) that further. There are a few of the tests which could be refactored into table tests if you feel strongly about it, but for now I am of the opinion that individual tests are always acceptable whereas table tests are sometimes acceptable, and not the other way around.

tldr: I think that table tests are less readable when debugging.

Here are some reasons why I didn't use a table test:

• each test is small enough for logic to be conceptualized at once
• avoided multiple functionality assertions in one test
• the tests do not requires the reader to jump around reading ancillary code
• My goal is to make it obvious why a test fails (often explicitly demonstrating what I do not care about in the test), not DRY.

This reasoning is from the experience I’ve had reading, writing, and maintaining tests and using unit tests to debug problems.

[na--]

@shirleyleu, in general, I agree with your reasons and this is a good summary:

individual tests are always acceptable whereas table tests are sometimes acceptable, and not the other way around.

However, you're missing the other side of the arguments, why table tests are sometimes better:

• modification of the tests (e.g. after the tested code is refactored) is much easier when it's in a single place
• when the test code for a whole bunch of very similar tests (with different inputs and expected outputs) is in a single place, you're much more sure that there aren't any mistakes in the tests themselves; adding a new test case is very unlikely to contain a copy-paste error or omission
• sometimes, reading and understanding table tests is actually easier - you might not care about the test minutia all that much, but seeing a "table" of inputs and expected outputs can help someone understand both the test and the tested code at a glance
• sub-tests allow you to separate and differentiate every case in a table test, so it's easier to debug any failures

In this case, it doesn't really matter and I'm fine with the current tests, they're ok as separate tests. However, the function names are a bit unidiomatic because of the underscores. In general, you should probably use TestNameWithMixedCaps. There are exceptions for related example tests, or sometimes to separate out related sub-tests, but then the underscores just separate out the MixedCaps parts (e.g. TestGetConsolidatedConfig_Succeeds, TestGetConsolidatedConfig_FromEnvVariables), not every word.

[fridgepoet]

Thanks for your feedback.
Respectfully, I’m not entirely convinced by all the points, but I’m happy to conform to the team’s/maintainer's style if it matters. (I’m not against table tests, though I didn’t follow through with this commit.)

If someone is just going to see this test file and vomit or have the irresistible urge to refactor, I can just do it now. Sorry @mstoykov that I didn’t just take the hint earlier.
Please let me know if you want me to refactor.

OK! I will change the test case names!

[na--]

If someone is just going to see this test file and vomit or have the irresistible urge to refactor, I can just do it now.

😄 No, as I mentioned, the tests are fine as separate ones, don't worry about it! And while @mstoykov would have probably done them differently, I don't think he'd have an irresistible urge to refactor them later 😝 I'm reading his comment as minor nitpick, at least

[fridgepoet]

Great, thanks! 😌

I've replaced the mostly snake case with mostly mixed case now. I've preserved the first underscore to match the case of the function names which they are testing.
Let me know if you want it fully mixed case no matter the case of the function name.

[mstoykov]

I have left some more comments about minor things, but I haven't run or tested this myself. Either way good job clap

Can you provide any numbers on how it behaves if you load it a lot?

Also maybe we should include the docker-compose.yml file(and the accompanying configuration) here as well ?

[fridgepoet]

Can you provide any numbers on how it behaves if you load it a lot?

Sure! ...uh sorry, what sort of scenarios and numbers should I try out?
Should I try some ridiculously small push interval?

[mstoykov]

Thanks for the PR @shirleyleu 🙇

[fridgepoet]

You’re welcome :) Sorry I didn’t finish the checklist but just message me if there’s any info I can help out with.

…

On Mon 13 Sep 2021 at 15:56 Mihail Stoykov ***@***.***> wrote: Thanks for the PR @shirleyleu <https://github.com/shirleyleu> 🙇 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7YLWV6ABJBUDZ32XTBYJTUBYGIZANCNFSM5BPHZUAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.