Move pageLoadTime to details

[Jyyjy]

While schematizing userale log data in a downstream application, I've noticed that pageLoadTime is the only unique field for a specific event. All other events use the details field, so it would make sense if pageLoadTime was also a detail.

[UncleGedd]

I'm totally cool with this, just want to make sure we aren't breaking any data pipelines for downstream users with this schema change

[brucearctor]

Ideally we'd have [ or later add ] tests for the downstream dependencies. Having on-hand, in repo, is an ideal way to facilitate developer velocity, and not having to check with users.

Related a published spec of what the project agrees to adhere to [ per major version, for example ] might be helpful?

Trying to ensure to not be in the business of having to check with downstream users - but to proactively be looking out for them, and establish on what the project can/not change, or at least without 'breaking'[major version] change.

[UncleGedd]

Ideally we'd have [ or later add ] tests for the downstream dependencies. Having on-hand, in repo, is an ideal way to facilitate developer velocity, and not having to check with users.

Related a published spec of what the project agrees to adhere to [ per major version, for example ] might be helpful?

Trying to ensure to not be in the business of having to check with downstream users - but to proactively be looking out for them, and establish on what the project can/not change, or at least without 'breaking'[major version] change.

100%. At this point, I'm not even sure how to verify (maybe @poorejc knows?). I think in the past when we've adjusted the schema it's just been a call out in the release notes. Maybe it's time to put that schema in code and version it?

[Jyyjy]

I could add a json schema, and some testcases to validate log data. I'm assuming the schema file should go in test/ and new test cases could be added to packageLogs_spec.js?

[brucearctor]

in this case, JSON Schema probably makes sense. Personally, generally, I try to avoid use of JSON Schema and err towards defining protobuf - but that is less prevalent in the web-focused world [ LOL -- though, at times, I even will send a proto over REST, and not using gRPC or gRPC Web.

Also curious, why add schema checks via an existing test file - rather than creating a new test file focused on tests specifically for schema?

If defining a schema that is being tested against, we might want that very clearly referenced/findable. That might be in /tests if well defined, but ensuring that is clearly published would potentially be valuable to users.

[Jyyjy]

On second thought, it would be cleaner to put schema tests in a new file. I suggested packageLogs_spec.js because the schema validation would probably test functions in packageLogs.js.

Another benefit of json schema over protobuf, json schema can be used by applications receiving userale data for safe deserialization.

[brucearctor]

In our case, I would not advocate for proto -- that is a higher learning curve, and engineering bar. JSON Schema suffices.

[poorejc]

IMHO, this makes a lot of sense. Jason is 100% pageload time does break the core schema. Moving to details makes a lot of sense, given the use of details.

Regarding downstream users, there are lots of folks using forks... so I don't feel bad making a smart breaking change. We could also configure the NPM release so that the package.json flag ~2.3.0 wont' grab 2.3.1 or 2.4.0 or however we version. That won't break existing builds.

[poorejc]

Update:

Been thinking about this more...

• No brainer to push PageLoad time to details
• I like the idea of schema validation, but thinking about this more, I think tests should focus on packageLogs and validating the core schema
• The thing that would truly break things is if we were too heavy handed in enforcing schemas coming out of the UserALE API--customLog, packageCustomLog are meant to be flexible. <--schema management for these should probably happen in downstream pipelines, as we do with ELK (soon Kafka).

I think the second two bullets are discussions we should have on user@flagon

[poorejc]

If nothing else heard, I'm good to merge this PR. I think its gtg (test looks good, and at least risky scope) then continue discussion on schema management as per above.

[brucearctor]

+1 Sorting out schema management not intimately tied to this PR

…

On Wed, Mar 15, 2023, 8:08 AM poorejc ***@***.***> wrote: If nothing else heard, I'm good to merge this PR. I think its gtg (test looks good, and at least risky scope) then continue discussion on schema management as per above. — Reply to this email directly, view it on GitHub <#347 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGMTJE64PCJTKRNIO3GW7TW4HLPTANCNFSM6AAAAAAVRN3YKI> . You are receiving this because you commented.Message ID: ***@***.***>

[poorejc]

Thanks, I'll do some more testing and get merged into Master! Thanks @Jyyjy

[poorejc]

@Jyyjy your PR is on master! Again, thanks so much for your thoughtful work here!

BTW, your PR is the very first commit to master for Apache Flagon as a TOP LEVEL PROJECT. Congrats! You're immutably committed into Flagon's project history 👍