Refactor AWS Kinesis exporter to allow for extended types of encoding #3655

MovieStoreGuy · 2021-05-27T03:46:12Z

Description: Moving to encoding type model for exporting data to kinesis while maintaining backwards comparability. Ref #1310

Effectively the same as #3434 with source branch changed.

Link to tracking Issue: We have split up the work needed for #1305 as per @owais' comments on the PR linked above.

Testing: I have added rather trivial tests to get this shipped and to make sure it doing what it says on the tin (so to speak).

Documentation: No behaviour changes are being made here, just a change in implementation detail. Any follow up PRs that extend the ability of this exporter will joined with documentation.

MovieStoreGuy · 2021-05-27T03:50:21Z

Super sorry to all those who are involved.

exporter/awskinesisexporter/encoding/encoder_jaeger_test.go

owais · 2021-05-27T13:27:21Z

exporter/awskinesisexporter/encoding/encoder.go

+
+// Encoder allows for the internal types to be converted to an consumable
+// exported type which is written to the kinesis stream
+type Encoder interface {


Encoder sounds like something that takes some data in, translates it to another format and returns the translated data but the implementation below also seems to be responsible for actually exporting data to kinesis. Can we rename this to something like KinesisForwarder or KinesisIngester.

I am actually borrowing this pattern from encoding/json.Encoder where you pass the writer to object and pass the types you want to encode to the writer.

Doing this meant that I wasn't breaking current behavour that was originally set for the exporter while allowing for future definitions to be added (coming once this has been added back into the contrib).

KinesisForwarder and KinesisIngester are not a good fit because that implies that the encoding and the actual kinesis producer implementation are tightly coupled and could lead to some avoidable code duplication. Furthermore, it is also breaks the stutter naming rule in Go.

Would it be fair to suggest EncodeWriter or EncodeProducer as the interface name?

I am actually borrowing this pattern from encoding/json.Encoder where you pass the writer to object and pass the types you want to encode to the writer.

Probably that's what is causing the conflict in my head :) When I saw encoder, I immediately associated it with ro expect an io.Writer somewhere. Seeing an awskinesis.Exporter was a bit surpising. Probably not a big deal.

KinesisForwarder and KinesisIngester are not a good fit because that implies that the encoding and the actual kinesis producer implementation are tightly coupled and could lead to some avoidable code duplication.
B

But we are already doing that by taking a dependency on awskinesis.Exporter. So naming it like that would only reflect the reality IMO.

Furthermore, it is also breaks the stutter naming rule in Go.

In that case it could be just Forwarder?

BTW do you expect to consume this package as a library somewhere else? If not, why not make the interface private? That way we can change it's name pretty easily in future if we feel the need. It'd also make this whole discussion a lot less important :)

Probably that's what is causing the conflict in my head :) When I saw encoder, I immediately associated it with ro expect an io.Writer somewhere. Seeing an awskinesis.Exporter was a bit surpising. Probably not a big deal.

Super sorry for the confusion, I just really like the method signature of: encoding.Jaeger(exporter).Encode(data).
Mainly because it is what I use to when dealing with JSON.

But we are already doing that by taking a dependency on awskinesis.Exporter. So naming it like that would only reflect the reality IMO.

Mmm, not wrong here. To help clarify my intent with this approach, I wanted to help keep backwards compatibility with jaeger while allowing new types of encoding / translations to be used. I am not too keen on using words such as Forwarder or Ingester because both don't infer that data is being mutated. I would prefer to keep it along the lines of Encoder or Translator.

I believe I have seen translator packages used for other packages that allow for converted data types being exported, would it be wise to follow that pattern?

BTW do you expect to consume this package as a library somewhere else? If not, why not make the interface private? That way we can change it's name pretty easily in future if we feel the need. It'd also make this whole discussion a lot less important :)

No :D My intent is to keep this internal to the kinesis exporter since it is rather tightly coupled on how the collector works.

OK. Let's make it internal then and we should be good.

I am not too keen on using words such as Forwarder or Ingester because both don't infer that data is being mutated. I would prefer to keep it along the lines of Encoder or Translator.

Is the data being mutated though? It is translated to another form. The original values are discarded by the exporter. I think most other exporters do the same. They convert internal data into the format they want to export in. They all call their internal implementations exporter or forwarder.

I just really like the method signature of: encoding.Jaeger(exporter).Encode(data).
Mainly because it is what I use to when dealing with JSON.

This is exactly what feels a bit strange to me. Looking this function call doesn't tell me that this involves a network operation. It looks like this thing is only tranlating bytes into another form. People will need to look at the implementation or read the docs to realize this is also sending the encoded data to kinesis.

That said, if we make it private, I can live with as we can change it later if we want to.

Alrighty, I have gone moved the package to be internal with a few other renames.

Sorry, what I meant was make them private in the same package by making sure the names start with lower case 😬 Sorry about the miscommunication.

I don't mind if you want to keep them in internal now that you've done the work though but not sure that the guidelines are about adding stuff to it. I think generally we use it to host code used by multiple packages in this project but something we don't want to support as part of the public API when using this repo as a library.

Aaaa, right! completely flew by me.

I think internal works because there is nothing external to this that would benefit from using the translate.ExportWriters besides this exporter.

owais · 2021-05-28T15:05:41Z

exporter/awskinesisexporter/factory.go

-	return Exporter{k, params.Logger}, nil
+	return Exporter{
+		awskinesis: k,
+		encoder:    encoding.Jaeger(k),


if kinesis is being directly passed to encoded, do we still need it as a field on the Exporter struct?

For this PR at least, I didn't want to change how much of the exporter would work and I felt like the start and shutdown operations fell out of scope for this PR.

The following PR is where I plan to address the actual kinesis exporter to allow for it to be more generic and allow for more types to be added.

owais · 2021-05-28T15:13:49Z

exporter/awskinesisexporter/encoding/encoder.go

+
+// Encoder allows for the internal types to be converted to an consumable
+// exported type which is written to the kinesis stream
+type Encoder interface {


I am actually borrowing this pattern from encoding/json.Encoder where you pass the writer to object and pass the types you want to encode to the writer.

Probably that's what is causing the conflict in my head :) When I saw encoder, I immediately associated it with ro expect an io.Writer somewhere. Seeing an awskinesis.Exporter was a bit surpising. Probably not a big deal.

KinesisForwarder and KinesisIngester are not a good fit because that implies that the encoding and the actual kinesis producer implementation are tightly coupled and could lead to some avoidable code duplication.
B

But we are already doing that by taking a dependency on awskinesis.Exporter. So naming it like that would only reflect the reality IMO.

Furthermore, it is also breaks the stutter naming rule in Go.

In that case it could be just Forwarder?

BTW do you expect to consume this package as a library somewhere else? If not, why not make the interface private? That way we can change it's name pretty easily in future if we feel the need. It'd also make this whole discussion a lot less important :)

MovieStoreGuy · 2021-05-31T03:13:51Z

hey @owais,

Are you happy that this is a reasonable state to be able to merge back to main?

owais

One nit-picky comment but looks good otherwise. Thanks

owais · 2021-06-01T09:43:00Z

exporter/awskinesisexporter/exporter.go

-				exportErr = err
-			}
-		}
+		e.logger.Error("Unable to write traces to kinesis", zap.Error(err))


I think the pipeline should report errors back to the user so instead of logging, you might want to wrap the error with a helpful message and return it.

Is it acceptable to just not log at all?

My intentions where to keep the original flow from the function and that was to log when there is an error and return the error.

Sorry, I reread what had written, @owais.

I had gone removed the logging and leaving it for the pipeline while implementing the todo that was originaly there.

MovieStoreGuy · 2021-06-04T00:08:17Z

I had a look at the failing checks, and it is an unrelated SignalFx test which isn't in scope in this PR.

Not sure why this would trigger that test to exceed its set threshold.

In order to make the transition from only jaeger traces to otlp data being written to kinesis, we've split up our approach and made sure it does not introduce breaking changes.

Adding in tests for the encoder to ensure the expected errors are being raised.

After some discussion on the PR, in order to keep things simple and easy to follow, encoding is moved to be internal/translate.

MovieStoreGuy · 2021-06-07T00:48:19Z

Sorry to poke you again @owais, I am keen to get this back into main :)

jpkrohling · 2021-06-07T07:27:18Z

This seems to be failing due to #3361 . Restarting.

MovieStoreGuy requested review from anuraaga and owais as code owners May 27, 2021 03:46

MovieStoreGuy requested a review from a team May 27, 2021 03:46

github-actions bot assigned mx-psi May 27, 2021

mx-psi reviewed May 27, 2021

View reviewed changes

exporter/awskinesisexporter/encoding/encoder_jaeger_test.go Outdated Show resolved Hide resolved

owais reviewed May 27, 2021

View reviewed changes

owais reviewed May 28, 2021

View reviewed changes

MovieStoreGuy force-pushed the smarciniak/add-encoders-to-awskinesisexporter branch from d0556fe to 3249b85 Compare May 31, 2021 00:11

owais reviewed Jun 1, 2021

View reviewed changes

MovieStoreGuy mentioned this pull request Jun 2, 2021

[kinesisexporter] Support publishing of multiple data types in the kinesis exporter. #1305

Closed

Sean ZO Marciniak and others added 9 commits June 4, 2021 15:42

Adding support for future dynmaic encoding

ac79c77

In order to make the transition from only jaeger traces to otlp data being written to kinesis, we've split up our approach and made sure it does not introduce breaking changes.

Adding in trivial tests for the jaeger encoder

066be96

Adding in tests for the encoder to ensure the expected errors are being raised.

Fixing linting issues

22bf1e5

Adding in license to files that are missing it

ff3604d

Fixing typo in godoc

8d45160

Moving translate package to be internal only.

11b33c6

After some discussion on the PR, in order to keep things simple and easy to follow, encoding is moved to be internal/translate.

Fixing left over imports

a6cf597

Addressing original TODO and removing log line.

b09fc5f

Fixing imports

f98da0e

MovieStoreGuy force-pushed the smarciniak/add-encoders-to-awskinesisexporter branch from 132c2fb to f98da0e Compare June 4, 2021 05:42

Aneurysm9 approved these changes Jun 7, 2021

View reviewed changes

mx-psi approved these changes Jun 8, 2021

View reviewed changes

owais approved these changes Jun 8, 2021

View reviewed changes

bogdandrutu merged commit dbfbffc into open-telemetry:main Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor AWS Kinesis exporter to allow for extended types of encoding #3655

Refactor AWS Kinesis exporter to allow for extended types of encoding #3655

MovieStoreGuy commented May 27, 2021

MovieStoreGuy commented May 27, 2021

owais May 27, 2021

MovieStoreGuy May 28, 2021 •

edited

Loading

owais May 28, 2021

MovieStoreGuy May 29, 2021

owais May 29, 2021

MovieStoreGuy May 30, 2021

owais May 31, 2021

MovieStoreGuy May 31, 2021

owais May 28, 2021

MovieStoreGuy May 29, 2021

owais May 28, 2021

MovieStoreGuy commented May 31, 2021

owais left a comment

owais Jun 1, 2021

MovieStoreGuy Jun 2, 2021

MovieStoreGuy Jun 3, 2021 •

edited

Loading

MovieStoreGuy commented Jun 4, 2021

MovieStoreGuy commented Jun 7, 2021

jpkrohling commented Jun 7, 2021

Refactor AWS Kinesis exporter to allow for extended types of encoding #3655

Refactor AWS Kinesis exporter to allow for extended types of encoding #3655

Conversation

MovieStoreGuy commented May 27, 2021

MovieStoreGuy commented May 27, 2021

Choose a reason for hiding this comment

MovieStoreGuy May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy commented May 31, 2021

owais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

MovieStoreGuy commented Jun 4, 2021

MovieStoreGuy commented Jun 7, 2021

jpkrohling commented Jun 7, 2021

MovieStoreGuy May 28, 2021 •

edited

Loading

MovieStoreGuy Jun 3, 2021 •

edited

Loading