Allow grouping all measurements into on with multiple fields #72
Conversation
Our organization needs to group all the fields under one measurement and transmit them as one measurement. This alternate serialization can be had by instantiating GroupedInfluxDbHttpSender instead of InfluxDbHttpSender. TODO: update unit tests to cover the new code.
|
@alorlea Thanks for reviewing. I'm looking for you comments, but I must be daft however, because there's none under your "view changes" button, and the "see review" button leads back here. Were your comments saved? |
| final byte[] line = influxDbWriteObjectSerializer.getLineProtocolString(influxDbWriteObject).getBytes(UTF_8); | ||
| String linestr = this.groupedFields | ||
| ? influxDbWriteObjectSerializer.getGroupedLineProtocolString(influxDbWriteObject,measurement) | ||
| : influxDbWriteObjectSerializer.getLineProtocolString(influxDbWriteObject); |
There was a problem hiding this comment.
As we already have a base abstract class, InfluxDbBaseSender.java it would be preferable to have each implementation to follow the SRP. As of now, this implementation has 2 underlying ways to function.
This logic should be part of the new GroupedInfluxDbHttpSender
|
@mnp Sorry, you are right, it seems that my comments where in a draft mode and forgot to submit. |
…class. This is cleaner.
|
Hi @mnp thank you for your changes! This looks better. Anything to add @nzroller @fredrikbackstrom ? |
|
@alorlea Sorry for sneaking in another commit a258730 after you looked. We just found that this reporter was not behaving consistently with other reporters like the Graphite one, with respect to the codahale JVM gauge group reporting. If this feature is controversial I would understand wanting to make it configurable. |
|
@mnp We checked over your commit, and we are fine with those changes. It could be great if you could add a brief explanation of how to enable this in the README? |
| * Passthrough to ultimately select a different style of serializer: grouped fields on one influxdb protocol line, | ||
| * instead of one field per protocol line. | ||
| */ | ||
| public class GroupedInfluxDbHttpSender extends InfluxDbHttpSender { |
There was a problem hiding this comment.
Could you write a bit extensive docs here, especially document the assumptions and limitations. (For example how tags are working).
After that this is ready for merge.
There was a problem hiding this comment.
Yes. Was on break, will do!
rickard-von-essen-iz
changed the title
|
Yeah we're actually using it, works for us. We don't use the factory, but
it's probably the right thing to include. I'll try to pack this up for
others.
…On Jan 23, 2018 11:48 PM, "Brandon Jernigan" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In metrics-influxdb/src/main/java/com/izettle/metrics/influxdb/
GroupedInfluxDbHttpSender.java
<#72 (comment)>
:
> @@ -0,0 +1,29 @@
+package com.izettle.metrics.influxdb;
+
+import java.util.concurrent.TimeUnit;
+
+import com.izettle.metrics.influxdb.data.InfluxDbWriteObject;
+import com.izettle.metrics.influxdb.utils.InfluxDbWriteObjectSerializer;
+
+/**
+ * Passthrough to ultimately select a different style of serializer: grouped fields on one influxdb protocol line,
+ * instead of one field per protocol line.
+ */
+public class GroupedInfluxDbHttpSender extends InfluxDbHttpSender {
@mnp <https://github.com/mnp> any luck getting this updated? This would
really help me out, thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#72 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA11BSiZ8wOl1CNWEDdiYaXrQSXppBELks5tNrYAgaJpZM4PB6Dt>
.
|
|
Where does this change sit? It appears that there needs to be more explanation of some of the code (as far as I can tell by the last comment). The person who initially made these changes has since left our company and I'd love to get this merged back into this library such that we don't have to have our own version of it. |
|
@tmousaw-ptc thanks for picking this up! I think we can merge this if you just add a short explanation how this works and a brief example of how to use it. See my #72 (comment) here. |
|
Folded in via another PR #116 |
This an attempt at #71.
Our organization needs to group all the fields under one measurement and transmit them as one measurement. This alternate serialization can be had by instantiating
GroupedInfluxDbHttpSenderinstead ofInfluxDbHttpSender.TODO: update unit tests to cover the new code.