Skip to content

chore(modulestore): add public coredump modulestore API #543

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

Closed
korniltsev wants to merge 1 commit into from
Closed

chore(modulestore): add public coredump modulestore API #543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tools/coredump/coredump_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ import (
"testing"

"github.com/stretchr/testify/require"
"go.opentelemetry.io/ebpf-profiler/tools/coredump/coredumpstore"
)

func TestCoreDumps(t *testing.T) {
cases, err := findTestCases(true)
require.NoError(t, err)
require.NotEmpty(t, cases)

store, err := initModuleStore()
store, err := coredumpstore.New()
require.NoError(t, err)

for _, filename := range cases {
Expand Down
74 changes: 74 additions & 0 deletions tools/coredump/coredumpstore/coredumpstore.go
View file
Open in desktop
Copy link
Copy Markdown
Member

@florianl florianl Jun 20, 2025

Choose a reason for hiding this comment

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

Can you elaborate a bit more, what you prevents from constructing your own modulestore.New() wrapper, so it can be used also in different places?

Would it be sufficient for you to move the constants, like moduleStoreRegion and others, to package modulestore and make them public?

Copy link
Copy Markdown
Contributor Author

@korniltsev korniltsev Jun 20, 2025

Choose a reason for hiding this comment

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

Can you elaborate a bit more, what you prevents from constructing your own modulestore.New() wrapper, so it can be used also in different places?

I don't quite get the question. This PR does exactly that.

Would it be sufficient for you to move the constants, like moduleStoreRegion and others, to package modulestore and make them public?

It would work. I still want a convenient wrapper, not just constants. I don't want to bother with aws config and s3 clients.

Copy link
Copy Markdown
Contributor Author

@korniltsev korniltsev Jun 20, 2025

Choose a reason for hiding this comment

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

I also want to have a shared cache

Copy link
Copy Markdown
Member

@florianl florianl Jun 20, 2025

Choose a reason for hiding this comment

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

From the request to use the coredump package, and its subpackages, in other places I would have expected some interface, that can be used and replaced in other situations.
That is why I did ask if the alternative with exposing the constants would be an option.

Personally, I don't have a good feeling around the findGitRoot() function. And so I'm looking for alternatives.

Copy link
Copy Markdown
Contributor Author

@korniltsev korniltsev Jun 20, 2025

Choose a reason for hiding this comment

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

just in case: I've added findGitRoot so that I don't have to specify the correct number of ../../../../../ for each different package that is going to use the new helper.

Copy link
Copy Markdown
Contributor Author

@korniltsev korniltsev Jun 27, 2025

Choose a reason for hiding this comment

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

Trying to understand you.

I propose to do this

package foo;

func TestName1(t *testing.T) {
	store, err := coredumpstore.New()
	require.NoError(t, err)

	id, err := modulestore.IDFromString("a621ea443bba20ffbdb5322633c5a1bf439905baa5822973b2cafc4106c64789")
	require.NoError(t, err)
	_ = store.UnpackModule(id, bytes.NewBuffer(nil))
	// ...
}

and you propose to do this

package foo;
// moduleStoreRegion defines the S3 bucket OCI region.
const moduleStoreRegion = "us-sanjose-1"

// moduleStoreObjectNamespace defines the S3 bucket OCI object name space.
const moduleStoreObjectNamespace = "axtwf1hkrwcy"

// modulePublicReadUrl defines the S3 bucket OCI public read only base path.
//
//nolint:lll
const modulePublicReadURL = "sm-wftyyzHJkBghWeexmK1o5ArimNwZC-5eBej5Lx4e46sLVHtO_y7Zf7FZgoIu_/n/axtwf1hkrwcy"

// moduleStoreS3Bucket defines the S3 bucket used for the module store.
const moduleStoreS3Bucket = "ebpf-profiling-coredumps"




func TestName1(t *testing.T) {
	localCachePath := "../../../../../tools/coredump/modulecache"
	publicReadURL := fmt.Sprintf("https://%s.objectstorage.%s.oci.customer-oci.com/p/%s/b/%s/o/",
		moduleStoreObjectNamespace, moduleStoreRegion, modulePublicReadURL, moduleStoreS3Bucket)

	cfg, err := config.LoadDefaultConfig(context.Background())
	require.NoError(t, err)

	s3Client := s3.NewFromConfig(cfg, func(o *s3.Options) {
		baseEndpoint := fmt.Sprintf("https://%s.compat.objectstorage.%s.oraclecloud.com/",
			moduleStoreObjectNamespace, moduleStoreRegion)
		o.Region = moduleStoreRegion
		o.BaseEndpoint = aws.String(baseEndpoint)
		o.UsePathStyle = true
	})
	store, err := modulestore.New(s3Client, publicReadURL, moduleStoreS3Bucket, localCachePath)
	require.NoError(t, err)

	id, err := modulestore.IDFromString("a621ea443bba20ffbdb5322633c5a1bf439905baa5822973b2cafc4106c64789")
	require.NoError(t, err)
	_ = store.UnpackModule(id, bytes.NewBuffer(nil))
	// ...
}

Right?

Copy link
Copy Markdown
Contributor Author

@korniltsev korniltsev Jun 27, 2025

Choose a reason for hiding this comment

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

And as there is only a single occurrence in the scope of this public repository, I think this should be fine.

That is what this PR is trying to change.

Copy link
Copy Markdown
Member

@florianl florianl Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for being short in #543 (comment) and not expanding more on the idea.

With #558 I have quickly implemented the idea that I proposed in #543 (comment).

With this, your tests should look like this:

func TestName1(t *testing.T) {
	localCachePath := "../../../../../tools/coredump/modulecache"
        cloudClient, err := cloudstore.Client()
	require.NoError(t, err)
	store, err := modulestore.New(cloudClient,
		cloudstore.PublicReadURL(), cloudstore.ModulestoreS3Bucket(), localCachePath)

Is this something you could work with?

Copy link
Copy Markdown
Contributor Author

@korniltsev korniltsev Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Your draft is almost the same as mine, why can't we make cloudstore.Client() just return the modulestore.Store ?

It technically work. It is less convenient. It is verbose and repetitive I still have to copypaste these details loudClient, cloudstore.PublicReadURL(), cloudstore.ModulestoreS3Bucket(), which I don't really care about ( I just need to pull a blob, I dont want to know about s3 clients and urls). It is fine to have them available as publi API for those who need them and needs to swap them. I just don't need them and propose something slightly more convenient.

It is ugly and inconvenitent to calculate the correct number of ../../../ localCachePath in each test.

Would that work for you if I add cloudstore.Client() , cloudstore.ModulestoreS3Bucket(), cloudstore.PublicReadURL() and then never use but still keep the cloudstore.New() with zero parameters?

Copy link
Copy Markdown
Member

@florianl florianl Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When thinking about APIs and packages, I try to separate them by their functionality.
To me, the most significant difference between #543 and #558 is, that #543 mixes functionality, while #558 separates them. Additionally, #558 also allows people to have their dedicated cloud module storage.

It is ugly and inconvenitent to calculate the correct number of ../../../ localCachePath in each test.

I agree with that. But I think, you can use and reuse your findGitRoot() from #543 to improve that.

Would that work for you [..] still keep the cloudstore.New() with zero parameters?

As pointed out, before, I think, packages and API should have their clear boundary and should be separated by functionality.

Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package coredumpstore // import "go.opentelemetry.io/ebpf-profiler/tools/coredump/coredumpstore"

import (
"context"
"errors"
"fmt"
"os"
"path/filepath"

"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/service/s3"
"go.opentelemetry.io/ebpf-profiler/tools/coredump/modulestore"
)

// moduleStoreRegion defines the S3 bucket OCI region.
const moduleStoreRegion = "us-sanjose-1"

// moduleStoreObjectNamespace defines the S3 bucket OCI object name space.
const moduleStoreObjectNamespace = "axtwf1hkrwcy"

// modulePublicReadUrl defines the S3 bucket OCI public read only base path.
//
//nolint:lll
const modulePublicReadURL = "sm-wftyyzHJkBghWeexmK1o5ArimNwZC-5eBej5Lx4e46sLVHtO_y7Zf7FZgoIu_/n/axtwf1hkrwcy"

// moduleStoreS3Bucket defines the S3 bucket used for the module store.
const moduleStoreS3Bucket = "ebpf-profiling-coredumps"

const localCachePath = "tools/coredump/modulecache"

// New creates a new modulestore.Store pointing to the public s3 used for coredump tests
func New() (*modulestore.Store, error) {
gitRoot, err := findGitRoot()
if err != nil {
return nil, err
}

localCachePath := filepath.Join(gitRoot, localCachePath)
publicReadURL := fmt.Sprintf("https://%s.objectstorage.%s.oci.customer-oci.com/p/%s/b/%s/o/",
moduleStoreObjectNamespace, moduleStoreRegion, modulePublicReadURL, moduleStoreS3Bucket)

cfg, err := config.LoadDefaultConfig(context.Background())
if err != nil {
return nil, err
}

s3Client := s3.NewFromConfig(cfg, func(o *s3.Options) {
baseEndpoint := fmt.Sprintf("https://%s.compat.objectstorage.%s.oraclecloud.com/",
moduleStoreObjectNamespace, moduleStoreRegion)
o.Region = moduleStoreRegion
o.BaseEndpoint = aws.String(baseEndpoint)
o.UsePathStyle = true
})
return modulestore.New(s3Client, publicReadURL, moduleStoreS3Bucket, localCachePath)
}

func findGitRoot() (string, error) {
it, err := os.Getwd()
if err != nil {
return "", err
}
for len(it) > 1 {
_, err = os.Stat(filepath.Join(it, ".git"))
if err == nil {
return it, nil
}
it = filepath.Dir(it)
}
return "", errors.New("git not found")
}
42 changes: 2 additions & 40 deletions tools/coredump/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,37 +11,18 @@ import (
"context"
"errors"
"flag"
"fmt"
"os"

"github.com/aws/aws-sdk-go-v2/aws"
awsconfig "github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/service/s3"

"github.com/peterbourgon/ff/v3/ffcli"
log "github.com/sirupsen/logrus"
"go.opentelemetry.io/ebpf-profiler/tools/coredump/modulestore"
"go.opentelemetry.io/ebpf-profiler/tools/coredump/coredumpstore"
)

// moduleStoreRegion defines the S3 bucket OCI region.
const moduleStoreRegion = "us-sanjose-1"

// moduleStoreObjectNamespace defines the S3 bucket OCI object name space.
const moduleStoreObjectNamespace = "axtwf1hkrwcy"

// modulePublicReadUrl defines the S3 bucket OCI public read only base path.
//
//nolint:lll
const modulePublicReadURL = "sm-wftyyzHJkBghWeexmK1o5ArimNwZC-5eBej5Lx4e46sLVHtO_y7Zf7FZgoIu_/n/axtwf1hkrwcy"

// moduleStoreS3Bucket defines the S3 bucket used for the module store.
const moduleStoreS3Bucket = "ebpf-profiling-coredumps"

func main() {
log.SetReportCaller(false)
log.SetFormatter(&log.TextFormatter{})

store, err := initModuleStore()
store, err := coredumpstore.New()
if err != nil {
log.Fatalf("%v", err)
}
Expand Down Expand Up @@ -70,22 +51,3 @@ func main() {
}
}
}

func initModuleStore() (*modulestore.Store, error) {
publicReadURL := fmt.Sprintf("https://%s.objectstorage.%s.oci.customer-oci.com/p/%s/b/%s/o/",
moduleStoreObjectNamespace, moduleStoreRegion, modulePublicReadURL, moduleStoreS3Bucket)

cfg, err := awsconfig.LoadDefaultConfig(context.Background())
if err != nil {
return nil, err
}

s3Client := s3.NewFromConfig(cfg, func(o *s3.Options) {
baseEndpoint := fmt.Sprintf("https://%s.compat.objectstorage.%s.oraclecloud.com/",
moduleStoreObjectNamespace, moduleStoreRegion)
o.Region = moduleStoreRegion
o.BaseEndpoint = aws.String(baseEndpoint)
o.UsePathStyle = true
})
return modulestore.New(s3Client, publicReadURL, moduleStoreS3Bucket, "modulecache")
}