Draft: Telemetry for v2 routes #202
Conversation
…olling back a secret to a version
| type: rawSecret.type, | ||
| user: new Types.ObjectId(req.user._id) | ||
| } | ||
|
|
||
| sanitizedSecretesToCreate.push(safeUpdateFields) | ||
| }) | ||
|
|
||
| const [bulkCreateError, newlyCreatedSecrets] = await to(Secret.insertMany(sanitizedSecretesToCreate).then()) | ||
| const [bulkCreateError, secrets] = await to(Secret.insertMany(sanitizedSecretesToCreate).then()) |
There was a problem hiding this comment.
Is there a Typo? Isn't the correct form is "sanitizedSecretsToCreate"
There was a problem hiding this comment.
I think it might be, but this controller is not user anymore. We will deprecate it soon, but for now the correct version is in secretsContoller.ts
| userAgent: req.headers?.['user-agent'] | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This PostHog boilerplate takes a lot of space and not easy to change in bulk in the future. Is it possible to seperate to function and use that function?
There was a problem hiding this comment.
Hmmm interesting! How would you do it? Personally I'd say it's already pretty concise, though it does take a lot of space
There was a problem hiding this comment.
What about we create a function to get distinctId, channel, userAgent from req and context variable which extends the default. Something like this;
type PostHogEventData = {
event: string
distinctId: string
properties: Record<any,any> & {
channel?: "web"|"cli"
userAgent?: string
}
}
function capturePostHogEvent(eventName: string, req: Request, context: Partial<PostHogEventData>){
if(!postHogClient) return;
const postHogData = Object.assign({
event: eventName,
distinctId: req.user.email,
properties: {
channel: req.headers?.['user-agent']?.toLowerCase().includes('mozilla') ? 'web' : 'cli',
userAgent: req.headers?.['user-agent']
}
}, context)
postHogClient.capture(postHogData);
}
...
capturePostHogEvent('secrets_added', req, {
properties: {
numberOfSecrets: 1,
workspaceId,
environment
}
})
There was a problem hiding this comment.
Agree with this!
I think we can add this as an extension to /services/PostHogClient in the backend. We can convert it to a class and have some useful methods on there like this one to capture events.
In the interest of time and un-blocking some people, we're going to merge this PR and add in this change in a separate PR. Feel free to submit a new PR with this modularization after the merge!
There was a problem hiding this comment.
There needs some changes to this PR and associated commits. There are some parts that we can optimize and some parts that we can seperate logics to their own functions for the purpose of easier to manage in the future. I would like to get your opininons on my reviews.
| secretIdsToDelete.forEach(secretIdToDelete => { | ||
| if (secretsUserCanDeleteSet.has(secretIdToDelete)) { | ||
| const deleteOperation = { deleteOne: { filter: { _id: new Types.ObjectId(secretIdToDelete) } } } | ||
| deleteOperationsToPerform.push(deleteOperation) | ||
| numSecretsDeleted++; | ||
| } else { | ||
| throw RouteValidationError({ message: "You cannot delete secrets that you do not have access to" }) |
There was a problem hiding this comment.
Won't this piece of code stop the process of deleting other secrets? Like if I want to delete 10 secrets in bulk but second secret in array throws this error, won't be the other remaining secrets not deleted? There is no redundancy for errors as well. Transactions and Bulk operations should follow Atomicity from ACID while working on databases.
Error Message should point to which of the secret(s) user do not have access to.
There was a problem hiding this comment.
Yeah, as mentioned above, I think this controller is about to be deprecated
There was a problem hiding this comment.
Yup this controller will be deprecated soon pretty much after the merge as we transition to the new endpoints in v2/secrets.
| secretIds: secrets.map((n: any) => n._id) | ||
| }); | ||
|
|
||
| readAction && await EELogService.createLog({ |
There was a problem hiding this comment.
Can this code be simplified and take advantage of Promises?
await EELogService.createActionSecret({
name: ACTION_READ_SECRETS,
userId: req.user._id.toString(),
workspaceId: workspaceId as string,
secretIds: secrets.map((n: any) => n._id)
})
.then((readAction)=>{
EELogService.createLog({
userId: req.user._id.toString(),
workspaceId: workspaceId as string,
actions: [readAction],
channel,
ipAddress: req.ip
});
})
.then(()=>{return res.status(200).json({secrets})})or just be entirely async without await, it will speed up response time because response is not dependent to Audit Logs success status.
There was a problem hiding this comment.
Ah yea we can do without await then because you're right response is not dependent on the audit log success status — Let's submit that in the next PR
| secretCommentCiphertext, | ||
| secretCommentIV, | ||
| secretCommentTag | ||
| } = secret; |
There was a problem hiding this comment.
I prefer to just use secret.xyz in required fields rather then deconstructing to seperate variables like here. It's just makes code hard to read because of the variables that used once and never again below code.
There was a problem hiding this comment.
Sounds good I'll make this adjustment after!
| router.post( | ||
| '/', | ||
| body('workspaceId').exists().isString().trim(), | ||
| body('environment').exists().isString().trim().isIn(['dev', 'staging', 'prod', 'test']), |
There was a problem hiding this comment.
So what about custom envrionment names?
There was a problem hiding this comment.
I think we don't have customer environment names yet. But agree, when we implement that we should change that
There was a problem hiding this comment.
Yeah Akhil will add these right after.
| throw new Error('secrets array must contain objects that conform to the Secret interface'); | ||
| } | ||
| } | ||
| } else if (typeof value === 'object') { |
There was a problem hiding this comment.
I have seen this check a lot in this PR. If we're gonna use this check over and over again we need to make it seperate function and use that function here. It's really not easy to manage in this scale.
There was a problem hiding this comment.
Sounds good. I'll throw this check into a helper function 👌
backend/src/routes/v2/secrets.ts
Outdated
|
|
||
| router.delete( | ||
| '/', | ||
| [ |
There was a problem hiding this comment.
Is this correct? The check is inside of Array but the Array is not deconstructed. Should the correct form be ...[check('secretIds')]
There was a problem hiding this comment.
Yeah I believe it's correct. I've tested it with and without correct secretIds
There was a problem hiding this comment.
Oh actually hmm maybe it would also work without the array..
Add endpoints for rollbacks and secret versions
No description provided.