logzio / logzio-azure-serverless

Azure function that ships Logs to logz.io
Apache License 2.0
10 stars 12 forks source link

Extension Bundle version and tmp path fix #60

Closed EmFl closed 2 years ago

EmFl commented 2 years ago

ExtensionBundle version was configured for 2.x which uses v4 of the eventhub libraries. Switching to 3.x to get version 5 which fixes an error about casting issue

The tmp fix is required because the temp storage can be either on C or D drive, since it was hardcoded, it failed on many function instances to create the tmp dir for blob storage.

I've also included a change for "FUNCTIONS_WORKER_PROCESS_COUNT" to use the default value of 1 since this seems to create a lot of chaos in our deployments. Having each function invocation with a single process count and letting azure handle the number of instances based on the triggers load seem to give a much smoother experience.

yotamloe commented 2 years ago

Hey, @EmFl thanks a lot for this contribution. Our team is working on migrating this project to use golang instead of nodeJS to improve performance. Ill take your notes regarding host.json and azuredeploylogs.json for the next release. Is it critical for you guys to merge this PR to main branch? can you use your branch for the custom deployment?

EmFl commented 2 years ago

Hey, We currently have between 10 to 15 active azure function deployment so I would prefer if someone from logz.io validates the change and confirm that it works as expected instead of pointing all of my functions (many of them in production) to a branch that I own on my personal github. But depending on the ETA for the next release, then we might be able to wait and then deploy directly the new function everywhere.

yotamloe commented 2 years ago

We plan to release a new version hopefully next week :) Is it relevant for you guys?

EmFl commented 2 years ago

Good news ! Sure we should be able to wait a week without any issue :) Thanks !

yotamloe commented 2 years ago

Great ill keep you posted once we have the PR ready

Doron-Bargo commented 2 years ago

Hi @EmFl Regrading FUNCTIONS_WORKER_PROCESS_COUNT Since the function is written is Node and JS is a single-threaded runtimes the best practice by Azure is to set it to 10 so it can run multiple function on the same host. I remember we had a lot of performance issues in the past and this was one of the recommendations to overcome those issues. Can you elaborate on the chaos it cause in your deployment?

yotamloe commented 2 years ago

Hi @EmFl Unfortunately we will not release the golang version this week. We still want to give you a solution you will be happy with, I tested the changes in this PR and everything seems to work as expected. We took your notes regarding FUNCTIONS_WORKER_PROCESS_COUNT and decided to add it as a parameter in the ARM template so users can choose what is working best for them. I opened #61 PR which includes your changes and the new functionsWorkerProcessCount parameter.

EmFl commented 2 years ago

Thanks for following up on this and for exposing the parameter as it's possible that depending on load or other factor this has various impacts. To answer @Doron-Bargo I sadly don't have the logs anymore to show this but with FUNCTIONS_WORKER_PROCESS_COUNT set to 10 and a function under high load we were facing function timeouts and error message related to checkpoint lease conflicts. A quick look at failed execution count confirms the amelioration following the change for us image

yotamloe commented 2 years ago

61 Is merged to master, closing this PR