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

feat: add execution id support #592

Merged

Conversation

liuyunnnn
Copy link
Contributor

@liuyunnnn liuyunnnn commented Feb 22, 2024

Add execution id support.

When set LOG_EXECUTION_ID=True in environment variables or set --log-execution-id, execution id will be added to log along with trace and span id.
Nodejs version 13.0.0 is required for feature above.

src/logger.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
@kenneth-rosario
Copy link
Contributor

I think you need to run npm run docs

Copy link
Contributor

@kenneth-rosario kenneth-rosario left a comment

Choose a reason for hiding this comment

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

Can we create integration test for this or would it be too complicated? similar to test/integration/http.ts . Not sure if we could redirect stdout to some sort of pipe and be able inspect the contents or make test function dump stdout on res.body. Let me know what you think.

src/execution_context.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
src/options.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
src/options.ts Outdated Show resolved Hide resolved
src/options.ts Outdated Show resolved Hide resolved
src/logger.ts Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/logger.ts Show resolved Hide resolved
@liuyunnnn liuyunnnn requested a review from nifflets February 28, 2024 17:22
@liuyunnnn
Copy link
Contributor Author

liuyunnnn commented Feb 28, 2024

Can we create integration test for this or would it be too complicated? similar to test/integration/http.ts . Not sure if we could redirect stdout to some sort of pipe and be able inspect the contents or make test function dump stdout on res.body. Let me know what you think.

I haven't figured out how to do that yet. All the ways I found are overriding console.log or process.stdout.write, which is the function we monkey patch in this PR.

README.md Outdated Show resolved Hide resolved
src/execution_context.ts Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
docs/generated/api.md Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
src/execution_context.ts Outdated Show resolved Hide resolved
kenneth-rosario
kenneth-rosario previously approved these changes Mar 5, 2024
Copy link
Contributor

@kenneth-rosario kenneth-rosario left a comment

Choose a reason for hiding this comment

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

LGTM, please wait on nifflets's approval before merging

src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@matthewrobertson matthewrobertson left a comment

Choose a reason for hiding this comment

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

Overall looks good. Lets drop the global getExecutionId function from the API for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants