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

Add log data type support #11

Merged
merged 2 commits into from
Nov 26, 2018
Merged

Add log data type support #11

merged 2 commits into from
Nov 26, 2018

Conversation

kkuzmin
Copy link

@kkuzmin kkuzmin commented Nov 20, 2018

Solution Description

The following features are added:

  • precompiled protobuf definitions for log data type.
  • helper function al_log:buildPayload() for converting incoming log messages to protobuf payloads ready to be transfered to Ingestion service.
  • HTTP request retry options. By default, two times retry for 5XX responses and system level errors.

Fix compile warnings.

Add compiled protos.

Switch to precompiled protobufjs. Update readme.

Add protobuf build helpers. Add request retry support.

Fix tests.

Readme typo fixes

PROTOC := $(shell which pbjs)

JS_PROTOS = alc_health.piqi_pb.js common_proto.piqi_pb.js dict.piqi_pb.js host_metadata.piqi_pb.js

Choose a reason for hiding this comment

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

if it is possible to not include compiled pb js (azure can compile it by itself), please remove it.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking of making a postinstall npm action but this will require too many magical scripting and looks like it doesn't worth it.
It is not guaranteed that the build machine has binutils or make installed - this is especially true for Azure where Azure deployment runs npm install on its own, potentially on Windows machine.
Anyway even if we have the postinstall script which compiles protobufs it still requires attention every time new protobuf file is added.
In general I think checking in precompiled protobufs is a safer and easier option for our use cases for both AWS Lambda and Azure Functions.

al_util.js Outdated
// randomize: false
minTimeout: 3000,
retries: 2,
maxTimeout: 30000

Choose a reason for hiding this comment

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

Isn't it too much of a delay for lambda?

Copy link
Author

Choose a reason for hiding this comment

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

Have just 2 retries here means that there will be one retry after 3 secs and another one after 9 secs.
By default, maxTimeout is set is Infinity. Just want to make sure that is anybody increases retries the maximum timeout would not exceed 30 secs.
Obviously we can change default retry strategy to something like:

    minTimeout: 1000,
    retries: 3,
    maxTimeout: 10000

Anyway the caller function is able to set its own retry strategy and here is just my best guess for default values. Any ideas are welcomed.

Choose a reason for hiding this comment

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

I think we can reduce such timeouts to smaller values as advised by @motobob to something like 300/600/900

Copy link
Author

Choose a reason for hiding this comment

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

Default retry timeouts changes to 300/2100/10000

@kkuzmin kkuzmin merged commit fc85bc5 into master Nov 26, 2018
@kkuzmin kkuzmin deleted the log branch November 26, 2018 10:01
daniel-greening pushed a commit to daniel-greening/al-collector-js that referenced this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants