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

feat(minor): Exporter specific configurations #286

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

Conversation

CrownedPhoenix
Copy link
Contributor

@CrownedPhoenix CrownedPhoenix commented Oct 4, 2024

Description

A series of refactors that culminate in enabling exporter specific configurations to be provided at benchmark initialization.

This is intended to support a use case like the following.

for n in [10, 100, 1000] {
    let x = Double.random(in: 0.0...100.0)
    Benchmark("Foo", configuration: .init(
        tags: ["n": String(n), "x": String(x)],
        exportConfigurations: [
            .influx: InfluxExportConfiguration(
                fields: ["x": .double]
            )
        ]
    )) { _ in
        foo(n, x)
    }
}

This PR is more easily reviewed by commit.

How Has This Been Tested?

I manually tested the exporter csv against an instance of Influx using Influx Data Explorer and querying a raw csv.
I followed instructions listed here

Minimal checklist:

  • I have performed a self-review of my own code
  • I have added DocC code-level documentation for any public interfaces exported by the package
  • I have added unit and/or integration tests that prove my fix is effective or that my feature works

Sorry, something went wrong.

@CrownedPhoenix CrownedPhoenix force-pushed the feat(minor)/exporter-specific-configurations branch from b577a14 to c0e674d Compare October 4, 2024 20:30
@CrownedPhoenix CrownedPhoenix changed the title Feat(minor)/exporter specific configurations feat(minor)/exporter specific configurations Oct 4, 2024
@CrownedPhoenix CrownedPhoenix changed the title feat(minor)/exporter specific configurations feat(minor): Exporter specific configurations Oct 4, 2024
@CrownedPhoenix
Copy link
Contributor Author

Would like some feedback on how/whether we'd prefer to do unit and/or integration tests for this feature.

@CrownedPhoenix CrownedPhoenix marked this pull request as ready for review October 4, 2024 20:34
@CrownedPhoenix CrownedPhoenix force-pushed the feat(minor)/exporter-specific-configurations branch from c0e674d to c1ba1a2 Compare October 4, 2024 20:51
Jairon Terrero added 5 commits December 5, 2024 15:07

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This was a redundant typealias

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Groups BenchmarkBaseline results by "profile" - meaning an execution of a specific benchmark and its various results (for each execution).
This allows us to store information pertinent to the entire benchmark in one place rather than repeating it for each BenchmarkResult.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This allows us to forward information about the benchmark down to the exporters.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@CrownedPhoenix CrownedPhoenix force-pushed the feat(minor)/exporter-specific-configurations branch from 4bc0a5d to b354858 Compare December 5, 2024 22:07
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 9.75610% with 37 lines in your changes missing coverage. Please review.

Project coverage is 68.86%. Comparing base (ec06262) to head (c080481).

Files with missing lines Patch % Lines
...tConfigurations/BenchmarkExportConfiguration.swift 8.11% 34 Missing ⚠️
...portConfigurations/InfluxExportConfiguration.swift 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   69.48%   68.86%   -0.62%     
==========================================
  Files          33       35       +2     
  Lines        3938     3979      +41     
==========================================
+ Hits         2736     2740       +4     
- Misses       1202     1239      +37     
Files with missing lines Coverage Δ
Sources/Benchmark/Benchmark.swift 69.47% <100.00%> (+0.16%) ⬆️
...portConfigurations/InfluxExportConfiguration.swift 0.00% <0.00%> (ø)
...tConfigurations/BenchmarkExportConfiguration.swift 8.11% <8.11%> (ø)
Files with missing lines Coverage Δ
Sources/Benchmark/Benchmark.swift 69.47% <100.00%> (+0.16%) ⬆️
...portConfigurations/InfluxExportConfiguration.swift 0.00% <0.00%> (ø)
...tConfigurations/BenchmarkExportConfiguration.swift 8.11% <8.11%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec06262...c080481. Read the comment docs.

@hassila
Copy link
Contributor

hassila commented Dec 7, 2024

Apologies for very late feedback, currently no bandwidth but will revert asap.

@hassila
Copy link
Contributor

hassila commented Feb 11, 2025

Overall LGTM, for integration testing I would consider to run and compare/verify a few of the public users of benchmarks in addition to your internal ones - should at some point create a verification suite using such sources, but outside of scope of this PR...

Some good candidates:
https://github.com/swiftlang/swift-foundation/tree/main/Benchmarks
https://github.com/Swift-CowBox/Swift-CowBox/tree/main/Benchmarks
https://github.com/apple/swift-homomorphic-encryption/tree/main/Benchmarks
https://github.com/wadetregaskis/Swift-Benchmarks

Just run and compare that things match up with main visavi PR.

WDYT?

@CrownedPhoenix
Copy link
Contributor Author

CrownedPhoenix commented Feb 11, 2025

Overall LGTM, for integration testing I would consider to run and compare/verify a few of the public users of benchmarks in addition to your internal ones - should at some point create a verification suite using such sources, but outside of scope of this PR...
[ ... ]
Just run and compare that things match up with main visavi PR.

WDYT?

I can do that for sure. Are you thinking just making sure their swift package benchmark still runs the same even if I swap out their dependency/checkout with this PR?

My assumption is that this is a non-breaking change and I can certainly confirm that this way.

Is there another comparison you have in mind besides just a continued, successful swift package benchmark execution?

@hassila
Copy link
Contributor

hassila commented Feb 12, 2025

If you are up for it, looking at adding a unit test or two for the export functionality would be great - otherwise verifying export in some other format as part of the above verification would be good. My assumption is also that this should be a non breaking change, thus the suggestion above. Also may consider adding a section to the DocC documentation on this functionality.

@hassila
Copy link
Contributor

hassila commented Feb 14, 2025

Would it make sense to address #227 while you are at it possibly?

@CrownedPhoenix
Copy link
Contributor Author

Happy to address that too. Seems like it should be a separate MR but I'll see if I can take care of it this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants