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

Split dice example into instrumented and uninstrumented #6300

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

IgorEulalio
Copy link

As discussed in this thread, we want to have two different examples, one with instrumentation and one without it:
#6296

@IgorEulalio IgorEulalio requested a review from a team as a code owner November 6, 2024 22:41
@pellared pellared added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Nov 7, 2024
@pellared
Copy link
Member

pellared commented Nov 7, 2024

Please fix the build.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.9%. Comparing base (b508e33) to head (f06e7d0).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6300   +/-   ##
=====================================
  Coverage   66.9%   66.9%           
=====================================
  Files        193     193           
  Lines      15652   15652           
=====================================
  Hits       10480   10480           
  Misses      4881    4881           
  Partials     291     291           

@IgorEulalio
Copy link
Author

@pellared ran locally and linter is now working.

@IgorEulalio
Copy link
Author

IgorEulalio commented Nov 7, 2024

@pellared uninstrumented didn't have the no-go lint for the rand.Int function, just added it.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Please also update .gitignore.

examples/dice/instrumented/main.go Outdated Show resolved Hide resolved
@IgorEulalio
Copy link
Author

done

@IgorEulalio
Copy link
Author

@pellared conflict here's due I removed go.mod file from examples directly while origin modified it, is there anything I need to do here?

@IgorEulalio
Copy link
Author

I merged the remote branch of the core repo into my fork

examples/dice/README.md Outdated Show resolved Hide resolved
examples/dice/README.md Outdated Show resolved Hide resolved
examples/dice/README.md Outdated Show resolved Hide resolved
examples/dice/README.md Outdated Show resolved Hide resolved
examples/dice/README.md Show resolved Hide resolved
examples/dice/README.md Outdated Show resolved Hide resolved
examples/dice/README.md Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Nov 12, 2024

@open-telemetry/docs-maintainers, is it something you want to help with maintaining https://github.com/open-telemetry/opentelemetry.io/blob/main/content/en/docs/languages/go/getting-started.md?

@svrnm
Copy link
Member

svrnm commented Nov 13, 2024

thanks for working on this @IgorEulalio, I will provide a review as soon as I can.

Note, that I recently proposed a project which is related to your work here, feel free to take a look and let me know if you'd like to participate: open-telemetry/community#2427

@IgorEulalio
Copy link
Author

@svrnm I took a look at the community issue, I would love to help and participate in this project. Do we already have something started on that?

@svrnm
Copy link
Member

svrnm commented Nov 18, 2024

@svrnm I took a look at the community issue, I would love to help and participate in this project. Do we already have something started on that?

No, nothing has started yet, I am currently seeing to find people who are interested in helping. So if you comment on that PR that you are interested in helping and in which capacity then I can add you to the Staffing

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, thanks for taking the time to make this! A few inline comments to align it better with what we need for the website

examples/dice/README.md Outdated Show resolved Hide resolved
examples/dice/instrumented/go.mod Show resolved Hide resolved
examples/dice/run.sh Show resolved Hide resolved
@chalin
Copy link

chalin commented Nov 18, 2024

…l shell scripts from docs

Signed-off-by: Igor Eulalio <[email protected]>
@svrnm
Copy link
Member

svrnm commented Nov 25, 2024

lgtm now (minus the conflicts that need to be resolved)

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Build is failing

examples/dice/instrumented/.tool-versions Outdated Show resolved Hide resolved
examples/dice/instrumented/get.sh Outdated Show resolved Hide resolved
examples/dice/instrumented/init.sh Outdated Show resolved Hide resolved
examples/dice/uninstrumented/run.sh Outdated Show resolved Hide resolved
examples/dice/run.sh Outdated Show resolved Hide resolved
examples/dice/instrumented/tidy.sh Outdated Show resolved Hide resolved
examples/dice/instrumented/run.sh Outdated Show resolved Hide resolved
@IgorEulalio
Copy link
Author

@pellared @svrnm I fixed the linter issue by removing the lint as it didn't make sense for me to the perfect appointment.

Let me know if you prefer to fix it trough code refactor.

@pellared
Copy link
Member

pellared commented Nov 26, 2024

I fixed the linter issue by removing the lint as it didn't make sense for me to the perfect appointment.

Let me know if you prefer to fix it trough code refactor.

I find it bad that the uninstrumented and instrumented code for this line does not match:

msg = player + " is rolling the dice"

and

msg = fmt.Sprintf("%s is rolling the dice", player) //nolint:perfsprint

I prefer the version without nolint (which is currently in main, I see no reason to change it).

@IgorEulalio
Copy link
Author

@pellared done

@pellared
Copy link
Member

pellared commented Nov 26, 2024

@pellared done

This #6300 (comment) is not resolved.

@IgorEulalio
Copy link
Author

This #6300 (comment) is not resolved.

Completely missed that one, it's fixed now.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

@svrnm, @chalin, can you please double-check?

examples/dice/instrumented/init.sh Show resolved Hide resolved
@pellared pellared changed the title Modify dice example so it has one instrumented and one uninstrumented example to highlight differences. Split dice example into instrumented and uninstrumented Nov 27, 2024
@pellared pellared requested a review from chalin November 27, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants