•  


feat(otel-instrumentation): Add custom instrumentation by psx95 · Pull Request #4149 · GoogleCloudPlatform/golang-samples · GitHub
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.

Sign up for GitHub

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

Jump to bottom

feat(otel-instrumentation): Add custom instrumentation #4149

Merged
merged 20 commits into from
Jun 3, 2024
Merged

feat(otel-instrumentation): Add custom instrumentation #4149

merged 20 commits into from
Jun 3, 2024

Conversation

psx95
Copy link
Contributor

Description

Updates the opentelemetry instrumentation example to showcase custom instrumentation - metrics & traces, along with exemplars.

Generated metrics also contain exemplars.

@psx95
Add custom metrics
53a99c3
@psx95
Switch custom metric from counter to histogram
d85a70b 
Histogram is better suited to showcase exemplars
@psx95
Update OTel collector version
bd204d8 
The older versions of collector contrib have a bug where integer valued
exemplars are dropeed.
@psx95
Add docstring to explain the doWork function
bd0124b
@psx95
Add custom tracing
dc14b48
@psx95 psx95 requested review from a team as code owners May 23, 2024 20:29
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label May 23, 2024
@psx95 psx95 changed the title Add manual instrumentation feat(otel-instrumentation): Add manual instrumentation May 23, 2024
@psx95 psx95 changed the title feat(otel-instrumentation): Add manual instrumentation feat(otel-instrumentation): Add custom instrumentation May 23, 2024
dashpole
dashpole reviewed May 23, 2024
View reviewed changes
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/server.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/server.go Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
aabmass
aabmass reviewed May 23, 2024
View reviewed changes
opentelemetry/instrumentation/app/work.go
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need region tags or the doc write up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, wanted thoughts on this PR to finalize the code before doc write-up. Can deal with region tags in a different PR.

opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
dashpole
dashpole reviewed May 24, 2024
View reviewed changes
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated
time.Sleep(sleepTime)

// record time slept
sleepHistogram.Record(r.Context(), int64(sleepTime/time.Millisecond), metric.WithAttributes(hostValue))
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is necessary given we have another histogram already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary, but I think its ok to have more than one metric - its in a different function, so probably won't be confusing to the users.

opentelemetry/instrumentation/app/server.go Outdated Show resolved Hide resolved
opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
@psx95
Revert the change to log message in /multi
6828417
@psx95
Update metric names & descriptions
2a2f401 
The updated names & descriptions follow the semantic conventions
dashpole
dashpole reviewed May 28, 2024
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

just needs location tags

opentelemetry/instrumentation/app/work.go Outdated Show resolved Hide resolved
@snippet-bot Snippet Bot
Copy link

snippet-bot bot commented Jun 3, 2024 •
edited

Here is the summary of changes.

You are about to add 2 region tags.

This comment is generated by snippet-bot .
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues .
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@psx95 psx95 merged commit 450027d into GoogleCloudPlatform : main Jun 3, 2024
9 checks passed
@psx95 psx95 deleted the add-exemplars branch June 3, 2024 17:58
Sign up for free to join this conversation on GitHub . Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass left review comments

@dashpole dashpole dashpole approved these changes

Assignees
No one assigned
Labels
samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet

3 participants
@psx95 @aabmass @dashpole
- "漢字路" 한글한자자동변환 서비스는 교육부 고전문헌국역지원사업의 지원으로 구축되었습니다.
- "漢字路" 한글한자자동변환 서비스는 전통문화연구회가 "울산대학교한국어처리연구실 옥철영(IT융합전공)교수팀"에서 개발한 한글한자자동변환기를 바탕하여 지속적으로 공동 연구 개발하고 있는 서비스입니다.
- 현재 고유명사(인명, 지명등)을 비롯한 여러 변환오류가 있으며 이를 해결하고자 많은 연구 개발을 진행하고자 하고 있습니다. 이를 인지하시고 다른 곳에서 인용시 한자 변환 결과를 한번 더 검토하시고 사용해 주시기 바랍니다.
- 변환오류 및 건의,문의사항은 juntong@juntong.or.kr로 메일로 보내주시면 감사하겠습니다. .
Copyright ⓒ 2020 By '전통문화연구회(傳統文化硏究會)' All Rights reserved.
 한국   대만   중국   일본 

Close