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

Update tutorials of both "Building a custom collector" and "Building a Trace Receiver" #3524

Merged

Conversation

brightzheng100
Copy link
Contributor

Re issue: #3488

This PR has two tutorials / files updated.

In "Building a custom collector":

  1. Added the folder structure to help readers understand where the files should be properly located.
  2. Added the further readings as the additional resources.

In "Building a Trace Receiver":

  1. Streamlined & simplified the development process;
  2. Applied the Go module / workspaces practices with a better folder structure;
  3. Fixed many broken links due to a missing v in the path. For example: /blob/v{{% param collectorVersion %}}/pdata/;
  4. Fixed other minor wording issues / typos.

@brightzheng100 brightzheng100 requested review from a team and codeboten and removed request for a team November 9, 2023 10:49
@svrnm
Copy link
Member

svrnm commented Nov 9, 2023

Hey @brightzheng100, thanks for taking the effort to improve those 2 documents, that's highly appreciated. I will try to provide you with a feedback soon.

@open-telemetry/collector-approvers please take a look as well!

@brightzheng100 brightzheng100 force-pushed the update-building-a-trace-receiver branch from 719f70a to 752b96a Compare November 9, 2023 12:17
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.

Overall it looks good to me. Please add telemetrygen to the cSpell:ignore list in line 5

@open-telemetry/collector-approvers PTAL!

content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
content/en/docs/collector/custom-collector.md Outdated Show resolved Hide resolved
@cartermp
Copy link
Contributor

@open-telemetry/collector-approvers please take a look!

@svrnm
Copy link
Member

svrnm commented Nov 22, 2023

I recently tried out the tracer receiver tutorial by using the updates from this PR and it worked just fine 👍

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks sane, but I didn't try this. Given that @svrnm did, I'm approving this, although I'd love to see the couple of changes I suggested.

content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
content/en/docs/collector/trace-receiver.md Outdated Show resolved Hide resolved
Now, create another folder called `tailtracer` so we can have a place to host
all of our receiver code.
You may use the
[telemetrygen](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/cmd/telemetrygen)
Copy link
Member

Choose a reason for hiding this comment

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

Should this URL be parameterized as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea but this URL might be stable enough to last for a while I believe.

Copy link
Member

Choose a reason for hiding this comment

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

can this conversation be resolved?

@svrnm svrnm added sig-approval-missing Co-owning SIG didn't provide an approval and removed sig-approval-missing Co-owning SIG didn't provide an approval labels Dec 7, 2023
@svrnm
Copy link
Member

svrnm commented Dec 15, 2023

@brightzheng100 can you fix the merge conflicts?

@brightzheng100
Copy link
Contributor Author

There are quite a lot of changes after my PR as it's been a while.

But I've tried to merge and test so you may have a final review and let me know if further changes are needed.

@cartermp
Copy link
Contributor

@brightzheng100 I think if you run npm run fix it should clear up the rest. The content looks great to me, though. When those issues get fixed I'll merge it.

@brightzheng100
Copy link
Contributor Author

Done!

I was seeing exceptions when running npm run fix, as attached below, so I stepped back by using only npm run fix:format and npm run serve to verify the contents before submitting.

But actually if I were to look into those exceptions, I should know how to fix those issues one by one based on the exception messages.

Not sure whether this is the right way like what I did (by looking into the exceptions) for contributors to review before git commit and git push, or there are some bugs in lint-md.

...
[10:52:14] Starting 'lint-md'...
/Users/brightzheng/workspaces/OSS/OpenTelemetry/opentelemetry.io-mine/content/en/docs/collector/custom-collector.md:162: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
/Users/brightzheng/workspaces/OSS/OpenTelemetry/opentelemetry.io-mine/content/en/docs/collector/building/receiver.md:201: MD034/no-bare-urls Bare URL used [Context: "https://github.com/rquedas/ote..."]
/Users/brightzheng/workspaces/OSS/OpenTelemetry/opentelemetry.io-mine/content/en/docs/collector/building/receiver.md:60: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
/Users/brightzheng/workspaces/OSS/OpenTelemetry/opentelemetry.io-mine/content/en/docs/collector/building/receiver.md:1121: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
[10:52:16] 'lint-md' errored after 1.83 s
[10:52:16] Error: Processed 364 files, 2 had issues.
    at Transform.<anonymous> (/Users/brightzheng/workspaces/OSS/OpenTelemetry/opentelemetry.io-mine/gulp-src/lint-md.js:152:15)
    at Transform.emit (node:events:526:35)
    at Transform.emit (node:domain:551:15)
    at endReadableNT (/Users/brightzheng/workspaces/OSS/OpenTelemetry/opentelemetry.io-mine/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:1003:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

@cartermp
Copy link
Contributor

Huh, interesting. Maybe it's a bug in the linting tool we use.

@cartermp cartermp merged commit cbedbcd into open-telemetry:main Dec 17, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants