Add Eventhub target #63
Conversation
| ehBatch[i] = ehEvent | ||
| } | ||
|
|
||
| if !eht.batching { // Defaults to using non-batch when nothing is provided |
There was a problem hiding this comment.
Is there a reason to support batching and single event emission? Why not just have batching as the only option here?
There was a problem hiding this comment.
Initially, I figured that batching would likely result in increased latency, which may be problematic for some use cases - which his how it is for the kafka target, where we support both.
Some initial rough tests have shown that this might not actually be the case - configuring it to send batches of 1 seems to be about the same as sending individual events. However I'm still not sure until I load test for it so I figured it's better to have both available than need to do a subsequent release to resolve an issue with just picking one.
pkg/target/eventhub.go
Outdated
| batching: cfg.Batching, | ||
| batchByteLimit: cfg.BatchByteLimit, | ||
|
|
||
| log: log.WithFields(log.Fields{"target": "eventhub", "cloud": "AWS", "namespace": os.Getenv("EVENTHUB_NAMESPACE"), "eventhub": os.Getenv("EVENTHUB_NAME")}), |
There was a problem hiding this comment.
Getenv is not always super safe as you might not get a legit value out - is there any risk that things will blow up if the env vars are not set? What kind of errors does stream replicator toss out if these values are empty?
There was a problem hiding this comment.
It will fail to initialise the client before it gets to this point if these env vars aren't set. The error thrown would look like this:
ERRO[0000] environment var EVENTHUB_NAMESPACE must not be empty error="environment var EVENTHUB_NAMESPACE must not be empty"
However, I recognise that this is outside the error handling pattern we have for all the other config options, so indeed perhaps it's best not to use env vars to initialise the client, and to handle these via the config object...
colmsnowplow
force-pushed
the
feat/eventHubs
branch
from
905bfc3
to
e8bd211
Compare
cmd/config.go
Outdated
| // FailureEventHubTargetConfig configures the destination for records consumed | ||
| type FailureEventHubTargetConfig struct { | ||
| EventHubNamespace string `env:"FAILURE_TARGET_EVENTHUB_NAMESPACE"` // REQUIRED - namespace housing Eventhub | ||
| EventHubName string `env:"FAILURE_TARGET_EVENTHUB_NAME"` // REQUIRED - namespace housing Eventhub |
There was a problem hiding this comment.
nit:
| EventHubName string `env:"FAILURE_TARGET_EVENTHUB_NAME"` // REQUIRED - namespace housing Eventhub | |
| EventHubName string `env:"FAILURE_TARGET_EVENTHUB_NAME"` // REQUIRED - name housing Eventhub |
pkg/target/eventhub.go
Outdated
| batching: cfg.Batching, | ||
| batchByteLimit: cfg.BatchByteLimit, | ||
|
|
||
| log: log.WithFields(log.Fields{"target": "eventhub", "cloud": "AWS", "namespace": cfg.EventHubNamespace, "eventhub": cfg.EventHubName}), |
There was a problem hiding this comment.
nit:
| log: log.WithFields(log.Fields{"target": "eventhub", "cloud": "AWS", "namespace": cfg.EventHubNamespace, "eventhub": cfg.EventHubName}), | |
| log: log.WithFields(log.Fields{"target": "eventhub", "namespace": cfg.EventHubNamespace, "eventhub": cfg.EventHubName}), |
pkg/target/eventhub.go
Outdated
| successes = messages | ||
| } | ||
|
|
||
| eht.log.Debugf("Successfully wrote %d/%d messages", len(successes), len(messages)) |
There was a problem hiding this comment.
We print this twice, here and line 98. This makes it look broken in the logs, like its emitting the events twice.
There was a problem hiding this comment.
Fair - I was mimicking the kinesis target, which prints one message per chunk, and one message when all chunks are complete. Changing to ensure that the messages are different for clarity.
|
I've noticed that if I don't set the Azure libraries Env Vars (such as key name and value), then the call to create the Event Hub simply hangs. This seems rather unfortunate and I can imagine a scenario where we spin this up and don't realise it's not processing events. |
|
Created an issue in the client library repo about that issue with it hanging. Azure/azure-event-hubs-go#230 Just adding tests for the new checks on env vars, should be good to go shortly. |
| _, azCertPathPresent := os.LookupEnv("AZURE_CERTIFICATE_PATH") | ||
| _, azCertPwrdPresent := os.LookupEnv("AZURE_CERTIFICATE_PASSWORD") | ||
|
|
||
| if !(connStringPresent || (keyNamePresent && keyValuePresent) || (tenantIDPresent && clientIDPresent && ((azCertPathPresent && azCertPwrdPresent) || clientSecretPresent))) { |
There was a problem hiding this comment.
Man, thats nasty. I hope they fix the hang in the library 😂 The ANDs and ORs look good to me here though 👍
colmsnowplow
force-pushed
the
feat/eventHubs
branch
from
f572d5b
to
000e254
Compare
PR for an eventHub target
I haven't managed to find an easily available option to mock an eventhub for testing... I notice that we don't have one for pubsub either - so I think I'll need some input on the best way to approach implementing some kind of unit or integration test for a target like this.