Fix bugs for 1.0.2 #369
Fix bugs for 1.0.2 #369
Conversation
| @@ -34,7 +34,7 @@ export interface DependencyTelemetry extends Telemetry { | |||
| /** | |||
| * Result code returned form the remote component. This is domain specific and can be HTTP status code or SQL result code | |||
| */ | |||
| resultCode: string; | |||
| resultCode: string | number; | |||
There was a problem hiding this comment.
Will we serialize it to string for JSON purposes, so that endpoint accepts the item?
There was a problem hiding this comment.
Yep! While converting to an Envelope it's converted to string.
I changed the type here because many pre-existing docs incorrectly assumed this to take a number. I figured it would be most beneficial to just actually fully support it as a number (before this it would silently fail as we don't expect number and would not convert to string).
| var fileName = new Date().getTime() + ".ai.json"; | ||
| var fileFullPath = path.join(directory, fileName); | ||
| this._getShallowDirectorySize(directory, (err, size) => { | ||
| if (err || size < 0) { |
There was a problem hiding this comment.
If the AI endpoint is inaccessible - will there be any perf issues in case we concurrently queue up a lot of storage operations such as calculate size of all files and then save a file? Or due to batching in SDK such calls will be rare and only happen couple times a minute, so no hit on storage throughput is expected?
There was a problem hiding this comment.
Batching is indeed the rate limit on this. On top of that, because this call is fully async (not blocking the event loop) I don't expect perf impact even if the batch size is configured to be very low.
| /** | ||
| * Stores the payload as a json file on disk in the temp directory | ||
| */ | ||
| private _storeToDisk(payload: any) { | ||
|
|
||
| //ensure directory is created | ||
| // tmpdir is /tmp for *nix and USERDIR/AppData/Local/Temp for Windows |
There was a problem hiding this comment.
Are we sure it's always USERDIR on Windows? I remember there were cases when %temp% defaults to "\Windows\Temp" where many users may have access. If it can end up in Windows\Temp - is there a way in node to give Windows permissions on directory such as ACL to protect it? If it can't - please disregard the comment.
There was a problem hiding this comment.
It's not guaranteed to always be USERDIR, the system configuration is used to get the temp directory. Here's the logic: https://github.com/nodejs/node/blob/master/lib/os.js#L121
path = process.env.TEMP ||
process.env.TMP ||
(process.env.SystemRoot || process.env.windir) + '\\temp';
if (path.length > 1 && path.endsWith('\\') && !path.endsWith(':\\'))
path = path.slice(0, -1);
So if the TEMP or TMP environment variable is changed, that new temp will be respected instead.
As for ACL: In node itself the chmod arguments we're already using is the API presented for changing permissions. Unfortunately the underlying libuv library that does this on node's behalf doesn't map chmod to ACL in a way that will set anything other than the readonly flag.
If we're interested in doing this beyond what node itself supports, we'll need native code(or a library that itself has native code). I found one library that does this, but it doesn't have the same amount of node version support that we have (and I'm not yet sure of the implications of taking a dependency on windows-only native code)
|
|
||
| if (foundOne) { | ||
| break; | ||
| let fileName = s[i] && typeof s[i].getFileName === 'function' && s[i].getFileName(); |
There was a problem hiding this comment.
This condition could be simplified to const fileName = s[i].getFileName(). getFileName should be always present on a CallSite object. We only need to check that filename is returned (in our case we seen undefined for functions defined inside eval).
There was a problem hiding this comment.
Good catch! Thanks!
These changes address the following issues: #362, #358, #346, #344, #368, #365
These changes also improve internal logging coverage in a few areas, and fix a few stability issues with retry caching from disk.