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 architecture diagram #26

Merged
merged 1 commit into from
Jan 14, 2025
Merged

feat: add architecture diagram #26

merged 1 commit into from
Jan 14, 2025

Conversation

GALLLASMILAN
Copy link
Contributor

@GALLLASMILAN GALLLASMILAN commented Jan 13, 2025

Signed-off-by: GALLLASMILAN <gallas.milan@gmail.com>
@GALLLASMILAN GALLLASMILAN marked this pull request as ready for review January 13, 2025 09:24
Copy link

@pilartomas pilartomas left a comment

Choose a reason for hiding this comment

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

Points below are only suggestions, the diagram is good enough to get the picture as is 👍

  • Given this is a production diagram, I'd expand the node-sdk part. It should be clear that there is an application running the OpenTelemetry Node SDK and the framework.
  • I would also omit the word "filter" in the collector as there is nothing to filter that's in the diagram's dataflow.
  • I would use word trace rather than span although spans are technically correct.

@GALLLASMILAN
Copy link
Contributor Author

  • node-sdk = The node-sdk could be part of the framework and the spans can be exported directly.
  • filter = There is a default filter on agents spans => This filter will be exposed in the collector in our stack. It's very important to implement it becouse of the performance.
  • I let the spans keyword, the trace is computed in the Observe part, but It should not be important for the architecture diagram.

@GALLLASMILAN GALLLASMILAN merged commit cde535e into main Jan 14, 2025
2 checks passed
@GALLLASMILAN GALLLASMILAN deleted the add-diagram branch January 14, 2025 07:17
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.

2 participants