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

feat: add asyncapi file birth timestamp in metrics collection. #1304

Merged
merged 25 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ spec-examples.zip
# Coverage for testing

coverage
.asyncapi-analytics
13 changes: 12 additions & 1 deletion src/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { existsSync } from 'fs-extra';
import { promises as fPromises } from 'fs';
import { v4 as uuidv4 } from 'uuid';

const { readFile, writeFile } = fPromises;
const { readFile, writeFile, stat } = fPromises;

class DiscardSink implements Sink {
async send() {
Expand Down Expand Up @@ -69,6 +69,7 @@ export default abstract class extends Command {

async recordActionMetric(recordFunc: (recorder: Recorder) => Promise<void>) {
try {
await this.setSource();
await recordFunc(await this.recorder);
await (await this.recorder).flush();
} catch (e: any) {
Expand All @@ -78,6 +79,16 @@ export default abstract class extends Command {
}
}

async setSource() {
const specFilePath = this.specFile?.getFilePath();
if (!specFilePath) { return; }
try {
const stats = await stat(specFilePath);
this.metricsMetadata['file_creation_timestamp'] = stats.birthtimeMs;
} catch (e: any) {
// do nothing.
Copy link
Member

Choose a reason for hiding this comment

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

What are the possible errors we could find here? File doesn't exist maybe? We don't have permission to access the file?

I have the feeling we should control (and report to New Relic) failures here.

Copy link
Member Author

@KhudaDad414 KhudaDad414 Apr 17, 2024

Choose a reason for hiding this comment

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

Since this function is ultimately being called from the finally method of the command with a file path, If there is any error with the file, We expect that the current call is actually reporting the error.
(strange sentence, hope it makes sense. 😆 )

clarified the comment in the code.

Copy link
Member

Choose a reason for hiding this comment

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

Just one question on this topic: if there is an error with the file, is it handled at the catch method as well, apart from the finally method? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@peter-rr if by "handled" you mean reporting to New Relic, then it is only being done in finally since it will run regardless of errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@KhudaDad414 @peter-rr It's generally considered best practice to handle errors as close to their source as possible. This helps maintain clear code structure and makes it easier to understand and reason about error handling logic, in this case having some exception type checks under the catch would help.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Amzani this is how I understand the CLI would work when an error happens:

  1. A CLI command is run against an invalid file path, or a file that the user doesn't have read permission etc...
  2. Error is thrown.
  3. catch method will log the error.
  4. finally method will report it to New Relic.

setSouce is being called from the finally method. so the same error that caused the finally method to run will occur again. If the error is already logged and is being reported to New Relic, I am failing to understand why it should be handled and not be ignored here.

Copy link
Member

Choose a reason for hiding this comment

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

I would need some clarification here: are we all talking about the catch method from the class, or the catch block inside the setSource method?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am saying that if there is an error with the file, we expect the command to throw an error, catch method inside the class will log the error, and finally method will report it.

Since The setSource is called from the finally method, it is unnecessary to do anything inside of it's catch block.

If we log it, we have logged it twice, if we initiate a report we have reported it twice.

One time by command trying to access the file and throwing the error and one time by us trying to access it here.

Hope it is clear now.

}
}
async finally(error: Error | undefined): Promise<any> {
await super.finally(error);
this.metricsMetadata['success'] = error === undefined;
Expand Down
Loading