feat: Allow struct field tags for influxdb with a marshaller #389
Conversation
- Adds marshal_struct_to_write_point.go - Adds marshal_struct_to_write_point_test.go
…o tag-marshal feat: adds MarshalStructToWritePoint function allowing users to pass a typed argument & get a *write.Point back - Adds MarshalStructToWritePoint function in marshal_struct_to_write_point.go - Adds marshal_struct_to_write_point_test.go
zob456
changed the title
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #389 +/- ##
==========================================
- Coverage 92.72% 92.05% -0.68%
==========================================
Files 23 24 +1
Lines 2242 2317 +75
==========================================
+ Hits 2079 2133 +54
- Misses 123 139 +16
- Partials 40 45 +5
☔ View full report in Codecov by Sentry. |
|
Thanks for the PR! I'll add this to our list to review and discuss the feature addition next week. Thanks again! |
powersj
changed the title
|
Thanks for the PR. I think we would be interested in landing this. One idea we thought about is it would be nice to get a unmarshaller as well, so you could do the full loop. I'll add this to the backlog to get you a full, proper review. |
|
@powersj great point! I hadn't thought of an Unmarshaller but that would be very helpful! I'll look out for the backlog issue! Thanks again! |
|
@powersj wanted to check back in & see if this is tracked somewhere as an issue so I can work on the additional functionality & open a proper PR? |
|
Hi @zob456, Ah I have the review of this PR tracked in the backlog for someone to provide a proper review. I do not have a public issue for the unmsarshaller, but feel free to put something up. Hopefully that answers your question. Thanks |
There was a problem hiding this comment.
Thank you for your PR. There are a few nits in the implementation, but it looks good. The primary question is: "Should this be part of the client library?".
With the functionality introduced herein, the client library would internally
- convert a struct to
write.Pointwith a help ofinfluxdbtag on struct fields (scope of this PR) - convert Point to lp.Metric
- encode lp metric to a protocol line and write protocol line(s)
I think that the conversion to a Point instance introduced herein is redundant ... the client library should better introduce a new WriteData function on WriteAPI as it was planned for a v3 version of this library. I would also prefer the approach taken in
influxdb-client-go/influxclient/writer.go
Line 152 in 91f06df
I think that this PR can introduce a good utility function outside the client library, but it should rather not become a new enhancement of the library.
| import ( | ||
| "errors" | ||
| "github.com/influxdata/influxdb-client-go/v2/api/write" | ||
| "log" |
There was a problem hiding this comment.
Pls use "github.com/influxdata/influxdb-client-go/v2/internal/log" as the rest of the client code does.
| // Tags is exported in case this is a type a user wants to use in their code | ||
| type Tags map[string]string | ||
|
|
||
| // Fields is exported in case this is a type a user wants to use in their code | ||
| type Fields map[string]interface{} |
There was a problem hiding this comment.
There is no need to export this, and if there is no need, we should better not. If it would have to be exported, it would be already done to support the Point struct.
| } | ||
| } | ||
|
|
||
| func checkEitherTagOrMeasurement(influxTag string) error { |
There was a problem hiding this comment.
a nit: this fn performs a slightly different thing to what the name suggests ... it should be better named checkFieldTag WDYT?
|
I would be fine with the tags and the overall approach that already went through review in a v3 client in influxdb-client-go/influxclient/writer.go Line 152 in 91f06df |
|
#394 adds the same functionality using struct tags that were planned for v3 version of this library |
Closes #
Proposed Changes
Briefly describe your proposed changes:
My suggested change creates a function in
apicalledMarshalStructToWritePointthat will take in a value of a custom type & return either a*write.Pointcorrectly formatted or anerrorThe company I work for works with Influx & I've had to write a custom marshaller for just this reason. I also found some other threads on stackoverflow & other places asking if this was on the road map / covered by another package.
I haven't contributed to a go package like this before so I hope I am in-line with best practices here.
Any feedback is greatly appreciated even if my PR is not accepted!
Thanks!
Checklist