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

Add CI Job to Run Benchmark and Publish Results #1312

Closed
Brandon-Kimberly opened this issue Jul 14, 2020 · 5 comments
Closed

Add CI Job to Run Benchmark and Publish Results #1312

Brandon-Kimberly opened this issue Jul 14, 2020 · 5 comments
Assignees
Labels
feature-request up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@Brandon-Kimberly
Copy link

Is your feature request related to a problem? Please describe.

Yes. Currently the script used to benchmark must be run manually. This can lead to out-of-date or missing benchmark performance data.

Describe the solution you'd like

I would like a new job added to the CI pipeline that would run the benchmark script, then publish the results as an artifact. This job could run on ever push or every release, or at some other interval.

Describe alternatives you've considered

The alternative is to keep running the script manually. As mentioned above, this may result in benchmark data being out-of-sync with the actual code. Automatically running the benchmark script and publishing the results will ensure that the two are always synchronized.

@dyladan dyladan added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jul 14, 2020
@sidharthv96
Copy link

I'd like to work on this.

Deno is pushing benchmarks to a git repo in their CI.

@dyladan Will a similar approach be enough? If so, any pointers to which repo should be used to push the data would be helpful.

@dyladan
Copy link
Member

dyladan commented Jul 27, 2020

Yes, I think a similar approach would be enough. I would push it to this repo.

@sidharthv96
Copy link

@dyladan Not sure if pushing to this repo will work from within CI, I'm not very familiar with CircleCI.
I think the data has to be maintained in a separate repo, because otherwise there will be merge conflicts right?

Currently benchmark data is exported as JSON and stored with historical data.
Kinda stuck on the pushing part though.

@dyladan
Copy link
Member

dyladan commented Jul 28, 2020

It should be implemented as a github action and can either be triggered by a master push, or a release publish. For an example of a release publish triggered build you can look at what we are doing for the docs right now at .github/workflows/docs.yml

@pichlermarc
Copy link
Member

done in #4144, benchmarking is now tracked by #4171

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
* feat(restify): add requestHook support

The `requestHook` config option allows custom span handling per request
layer.

* fix(restify): pass config to super class

As mentioned in the review, pass the instrumentation config to the
parent class. That way the config is also stored when given to the
initializer, rather only when using the `setConfig` function.

* fix(restify): fix comment referencing restify type

Update comment to reference to correct type from the `@types/restify`
package.

* fix(restify): import missing Span type

Add the missing import reported by the linter.

* fix(restify): fix issues reported by linter

* fix(restify): fix comment referencing restify type

Mention the package name exactly.

* fix(restify): fix comment referencing restify type

Co-authored-by: Amir Blum <[email protected]>

* fix(restify): add missing import in restify test

* feat(restify): add layer argument to requestHook

Add the layerType argument to the requestHook function. This is like the
following PR but for restify:
open-telemetry/opentelemetry-js-contrib#1226

Move the LayerType from internal-types to types, because it's now used
in a function used by users of the instrumentation package.

Co-authored-by: Amir Blum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants