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

[FAI-13519] - GitHub Copilot usage recordedAt timestamp #1753

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

crupley
Copy link
Contributor

@crupley crupley commented Oct 21, 2024

Description

Populates the recordedAt field on vcs_UserToolUsage from the emitted_at timestamp on the source.

Type of change

  • Bug fix
  • New feature
  • Breaking change

@@ -34,6 +34,7 @@ Array [
},
Object {
"vcs_UserToolUsage": Object {
"recordedAt": "2024-06-20T09:54:42.836Z",
Copy link
Contributor Author

@crupley crupley Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this manually to match the value in the streams file; is there a better way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to build before running converter tests:
npm run build

Run faros_github converter tests and pass the flag to update the snapshot:
npm run test faros_github -- -u

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed that npm run test faros_github -- -u yields the same snapshot file as what's here

@@ -80,6 +80,7 @@ export class FarosCopilotSeats extends GitHubConverter {
record: {
userTool,
usedAt: Utils.toDate(activeSeat.last_activity_at),
recordedAt: Utils.toDate(record.record.emitted_at),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to use the emitted_at field from the stream here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can safely assume that field is present (it's written by the CDK)
It represents the point in time at which the record was emitted by the source

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what I want to capture here. But maybe I should have asked: Is it safe to use a system field like this or would it be better to write a value from the source as part of the record data and pick that up in the destination?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't bother if it's effectively $now

Copy link

sonarcloud bot commented Oct 21, 2024

@crupley crupley merged commit e2e58fb into main Oct 21, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants