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

OpenTelemetry Protocol with Apache Arrow in Production #5198

Merged
merged 32 commits into from
Sep 25, 2024
Merged

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Sep 13, 2024

Fixes #5193.

@opentelemetrybot opentelemetrybot requested a review from a team September 13, 2024 21:43
@github-actions github-actions bot added the blog label Sep 13, 2024
@jmacd jmacd marked this pull request as ready for review September 13, 2024 21:48
@jmacd jmacd requested a review from a team September 13, 2024 21:48
Copy link
Contributor

@tiffany76 tiffany76 left a comment

Choose a reason for hiding this comment

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

First pass edits - had to stop around line 92. Will do more next week! Thanks.

content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Thanks for writing this, great read. My main question is around "OTAP" vs "OTel-Arrow", are those 2 things different? So is it "OTel-Arrow Receiver" or "OTAP Receiver" like "OTLP receiver".

Also, a question on where this content should live: there is a lot of practical material in the content, that looks more like docs dann a blog. Is OTel Arrow ready for production such that we can take some of that into the documentation (e.g. within the collector docs)?

content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
Co-authored-by: Tiffany Hrabusa <30397949+tiffany76@users.noreply.github.com>
@opentelemetrybot opentelemetrybot requested a review from a team September 16, 2024 16:47
@jmacd
Copy link
Contributor Author

jmacd commented Sep 16, 2024

@svrnm About the length --
I have cut the section "Batching and back-pressure in OTel-Arrow streams", moved it to the exporter documentation: open-telemetry/opentelemetry-collector-contrib#35225

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Joshua

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

lgtm overall, most comments are suggestions. There is only one around the "collaboration with F5" wording, see my inline comment

content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
content/en/blog/2024/otel-arrow-production/index.md Outdated Show resolved Hide resolved
Co-authored-by: Severin Neumann <neumanns@cisco.com>
@opentelemetrybot opentelemetrybot requested a review from a team September 17, 2024 20:50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember changing this file. Is it OK?

@tiffany76
Copy link
Contributor

Hi @jmacd, thanks for your patience. Most of our maintainers are out of the office at the moment. Once they're back, we'll work to get this blog post merged ASAP.

As for the submodule file, it should not be included with the PR. If you're able to remove the changes locally, please go ahead. If that's not possible, the docs maintainers will know how to handle the unintended changes.

@svrnm
Copy link
Member

svrnm commented Sep 24, 2024

As for the submodule file, it should not be included with the PR. If you're able to remove the changes locally, please go ahead. If that's not possible, the docs maintainers will know how to handle the unintended changes.

You should be able to fix this by npm run sync locally, but we can also fix it here before merging.

@jmacd
Copy link
Contributor Author

jmacd commented Sep 24, 2024

npm run sync appeared to work.
npm run check locally has some errors for me, like

tmp/bin/htmltest --log-level 1
htmltest started at 09:18:56 on public
========================================================================
running in concurrent mode, this is experimental
ja/docs/concepts/sampling/index.html
  target does not exist --- ja/docs/concepts/sampling/index.html --> /docs/specs/otel/trace/tracestate-probability-sampling-experimental/#consistent-probability-sampling

@svrnm
Copy link
Member

svrnm commented Sep 24, 2024

npm run sync appeared to work.

Perfect.

npm run check locally has some errors for me, like

That's unrleated to your PR unfortunately. We have turned on merge queues recently and apparently didn't set it up properly, so CI is running but not failing and still merging PRs with errors. Ignore it for now.

@svrnm
Copy link
Member

svrnm commented Sep 24, 2024

/fix:all

@opentelemetrybot
Copy link
Collaborator

You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/11020834246

@opentelemetrybot
Copy link
Collaborator

fix:all failed or was cancelled. For details, see https://github.com/open-telemetry/opentelemetry.io/actions/runs/11020834246.

@svrnm
Copy link
Member

svrnm commented Sep 24, 2024

/fix:submodules

@opentelemetrybot
Copy link
Collaborator

You triggered fix:submodules action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/11020902472

@opentelemetrybot
Copy link
Collaborator

fix:submodules failed or was cancelled. For details, see https://github.com/open-telemetry/opentelemetry.io/actions/runs/11020902472.

@opentelemetrybot opentelemetrybot requested a review from a team September 25, 2024 06:58
@opentelemetrybot opentelemetrybot requested a review from a team September 25, 2024 06:59
@svrnm svrnm added this pull request to the merge queue Sep 25, 2024
Merged via the queue into main with commit d3f564f Sep 25, 2024
19 checks passed
@svrnm svrnm deleted the jmacd/otelarrowprod branch September 25, 2024 07:07
@svrnm
Copy link
Member

svrnm commented Sep 25, 2024

@jmacd I fixed the issue with the git submodules. It took me a few tries, so apologies for the commit & message pollution.

Eventually the solution was:

  • merge with main
  • run npm run sync locally to have the git submodules updated
  • commit that change (!), otherwise in the next step this will be reverted once again
  • run npm fix:all to apply any other missing changes / verify that all checks are green

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

Successfully merging this pull request may close these issues.

New Blog Post: OpenTelemetry Protocol with Apache Arrow in Production
6 participants