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

Storage Extension 1/N #2883

Merged
merged 12 commits into from
Apr 2, 2021
4 changes: 4 additions & 0 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ updates:
directory: "/extension/observer/k8sobserver"
schedule:
interval: "weekly"
- package-ecosystem: "gomod"
directory: "/extension/storage"
schedule:
interval: "weekly"
- package-ecosystem: "gomod"
directory: "/internal/aws"
schedule:
Expand Down
41 changes: 41 additions & 0 deletions exporter/stackdriverexporter/go.sum

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions extension/storage/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include ../../Makefile.Common
19 changes: 19 additions & 0 deletions extension/storage/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Storage

A storage extension persists state beyond the collector process. Other components can request a storage client from the storage extension and use it to manage state.

The `storage.Extension` interface extends `component.Extension` by adding the following method:
```
GetClient(component.Kind, configmodels.NamedEntity) (Client, error)
```

The `storage.Client` interface contains the following methods:
```
Get(string) ([]byte, error)
Set(string, []byte) error
Delete(string) error
```
Note: All methods should return error only if a problem occurred. (For example, if a file is no longer accessible, or if a remote service is unavailable.)

# TODO Sample code
- Document component expectations
10 changes: 10 additions & 0 deletions extension/storage/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module github.com/open-telemetry/opentelemetry-collector-contrib/extension/storage

go 1.14

require (
github.com/stretchr/testify v1.7.0
go.etcd.io/bbolt v1.3.3
go.opentelemetry.io/collector v0.23.0
go.uber.org/zap v1.16.0
)
1,408 changes: 1,408 additions & 0 deletions extension/storage/go.sum

Large diffs are not rendered by default.

29 changes: 29 additions & 0 deletions extension/storage/nop_client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package storage

type NopClient struct{}
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the model we have in core. and provide a func NewNopClient() Client and avoid exposing the new struct.

May also want to have this in a storagetest package, can be done later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this to follow the pattern established in core. However, I have a question about potentially keeping NewNopClient available.

One of the expectations is that components that make use of a storage extension should seamlessly handle the situation where no storage extension is configured. My intention with introducing this nopClient was to provide an easy way for components to fulfill this expectation. Consider:

// Start the component
func (c *myComponent) Start(ctx context.Context, host component.Host) error {
    client := getStorageClient(ctx, host)
    
    // component implementation uses client but doesn't care if it's nop or not
}


func getStorageClient(ctx context.Context, host component.Host) storage.Client {
    for _, ext := range host.GetExtensions() {
        if ext is a storage extension {
            return ext.GetClient()
        }
    }
    return storage.NewNopClient()
}

Do you think this is a useful pattern? I suppose components can implement their own nopClient if necessary, but I thought it might be helpful to include.

Copy link
Member

Choose a reason for hiding this comment

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

Let's see in practice how this will be used in few examples, probably we should make sure we re-evaluate this after couple of PRs that uses it.


func (c NopClient) Get(string) ([]byte, error) {
return nil, nil // no result, but no problem
}

func (c NopClient) Set(string, []byte) error {
return nil // no problem
}

func (c NopClient) Delete(string) error {
return nil // no problem
}
34 changes: 34 additions & 0 deletions extension/storage/storage.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package storage

import (
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configmodels"
)

// Extension is the interface that storage extensions must implement
type Extension interface {
component.Extension
GetClient(component.Kind, configmodels.NamedEntity) (Client, error)
djaglowski marked this conversation as resolved.
Show resolved Hide resolved
djaglowski marked this conversation as resolved.
Show resolved Hide resolved
}

// Client is the interface that storage clients must implement
// All methods should return error only if a problem occurred
type Client interface {
Get(string) ([]byte, error) // returns nil, nil if not found
djaglowski marked this conversation as resolved.
Show resolved Hide resolved
djaglowski marked this conversation as resolved.
Show resolved Hide resolved
Set(string, []byte) error
Delete(string) error
}