feat: add asyncapi file birth timestamp in metrics collection. #1304
Conversation
KhudaDad414
changed the title
KhudaDad414
requested review from
Souvikns,
magicmatatjahu,
derberg and
asyncapi-bot-eve
as code owners
src/base.ts
Outdated
| @@ -3,7 +3,7 @@ import { MetadataFromDocument, MetricMetadata, NewRelicSink, Recorder, Sink, Std | |||
| import { Parser } from '@asyncapi/parser'; | |||
| import { Specification } from 'models/SpecificationFile'; | |||
| import { join, resolve } from 'path'; | |||
| import { existsSync } from 'fs-extra'; | |||
| import { existsSync, statSync } from 'fs-extra'; | |||
There was a problem hiding this comment.
Why not using the asynchronous versions of these functions? 🤔 You can easily make setSource an async function and then it would be easy to call await stat() instead.
There was a problem hiding this comment.
Working fine with: const stats = await fPromises.stat(specFilePath);
Don't know if there is any better alternative to make setSource an async function.
There was a problem hiding this comment.
@peter-rr I don't think there is any. we either have to use promises and await it. or use Sync functions. which I guess the first option is a lot better.
|
@fmvilas need your approval to merge this. |
src/base.ts
Outdated
| const stats = await stat(specFilePath); | ||
| this.metricsMetadata['file_creation_timestamp'] = stats.birthtimeMs; | ||
| } catch (e: any) { | ||
| // do nothing. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@Amzani this is how I understand the CLI would work when an error happens:
- A CLI command is run against an invalid file path, or a file that the user doesn't have read permission etc...
- Error is thrown.
catchmethod will log the error.finallymethod 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.
There was a problem hiding this comment.
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.
|
@KhudaDad414 can you elaborate more on how this will partially solve #1300 thanks |
|
@Amzani the so after merging this PR we can close the issue. |
|
@fmvilas waiting for your review. 🙇♂️ |
|
/rtm |
|
Hello, @Amzani! 👋🏼 |
|
|
🎉 This PR is included in version 1.13.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
/update |
Description
fixes #1225, fixes #1161, fixes partialy #1300
EDIT:
fixes #1300
cc: @Amzani @smoya @peter-rr