Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

derived_tstamp and etl_tstamp should always be defined #80

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

istreeter
Copy link
Contributor

@istreeter istreeter commented Apr 18, 2024

Previously, this app generated events with undefined etl_tstamp and derived_tstamp, even when enrichments are enabled. This old behaviour was defensible because the purpose of this app is to generate all edge cases of possible events.

However, if you look at the Enrich code, it is completely impossible for those timestamps to be null:

  • etl_tstamp is set here
  • For derived_tstamp, see these comments to see why it is always set.

I think it is reasonable to say a valid enriched event must have non-null etl_tstamp and derived_tstamp. Then this event-generator can be used for testing downstream tooling that depend on those fields.

@colmsnowplow
Copy link
Contributor

colmsnowplow commented Apr 18, 2024

I think ideally these values should be omitted for http target - trackers won't ever set them when sending to the collector. I can't remember the implementation well enough to say what the behaviour currently is.

Quite likely when we have them we send them currently, so I don't think I'd block this on that ask, but if it's easy to slap a conditional on this change then why not

@istreeter
Copy link
Contributor Author

It's a good point, but I checked and its ok: this change does not affect what gets sent to the http target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants