[OTel]: enable persistence by default in our OTel distribution #5347

VihasMakwana · 2024-08-23T09:53:44Z

Enhancement

What does this PR do?

By default, the filelog receiver stores offsets in memory rather than on disk. Since users might forget to enable on-disk storage themselves, this PR enables persistence by default in our sample configurations.

Why is it important?

If the collector is restarted, there could either be data duplication (if it starts re-reading files from the beginning) or data loss (if it starts reading files from the end and new data has been written to these files during the restart).

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

Disruptive User Impact

How to test this PR locally

Pull from this branch.
Run elastic-agent otel
Try ingesting some data
Shutdown the otel command
Rerun the elastic-agent otel
Write new data
You shouldn't see duplicated data, as offsets will be stored on disk.

Related issues

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

mergify · 2024-08-23T09:54:25Z

This pull request does not have a backport label. Could you fix it @VihasMakwana? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

changelog/fragments/1724406973-enable-persistence-by-default.yaml

elasticmachine · 2024-08-23T10:01:41Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

mergify · 2024-08-23T10:05:29Z

This pull request does not have a backport label. Could you fix it @VihasMakwana? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elastic-sonarqube · 2024-08-23T13:06:10Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
73.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

VihasMakwana · 2024-08-29T07:48:58Z

@elastic/elastic-agent-control-plane can someone take a look here?

strawgate · 2024-08-29T13:56:44Z

Do we also need to update the sample yaml configurations that onboarding uses?

ycombinator · 2024-08-29T14:40:29Z

Do we also need to update the sample yaml configurations that onboarding uses?

Good catch. I think we should. They live here: https://github.com/elastic/elastic-agent/tree/main/internal/pkg/otel/samples

VihasMakwana · 2024-08-29T14:58:35Z

@strawgate @ycombinator those configs already use the filestorage extension.
For eg.

elastic-agent/internal/pkg/otel/samples/linux/platformlogs_hostmetrics.yml

Lines 1 to 9 in fd477ec

    
           receivers: 
        
             # Receiver for platform specific log files 
        
             filelog/platformlogs: 
        
               include: [ /var/log/*.log ] 
        
               retry_on_failure: 
        
                 enabled: true 
        
               start_at: end 
        
               storage: file_storage 
        
           #   start_at: beginning

strawgate · 2024-08-29T15:03:24Z

It seems like STORAGE_DIR is probably too general to be using as an environment variable here?

If this PR changes the default then can these changes be removed from the onboarding yaml?

VihasMakwana · 2024-08-29T15:19:03Z

@strawgate

If this PR changes the default then can these changes be removed from the onboarding yaml?

Not really,

If you take a look here

elastic-agent/internal/pkg/otel/run.go

Lines 91 to 99 in f32e874

    
           storageDir := os.Getenv("STORAGE_DIR") 
        
           if storageDir == "" { 
        
           	// by default use "${path.data}/registry/otelcol" to store offsets 
        
           	storageDir = filepath.Join(paths.Data(), "registry", "otelcol") 
        
           	// set the STORAGE_DIR env. This will be used by otel collectore to get the registry directory 
        
           	os.Setenv("STORAGE_DIR", storageDir) 
        
           }

I only override STORAGE_DIR if it's not set by the user (by default I use elastic-agent/data/registry/otelcol, but we can change the default).

If a user has set the STORAGE_DIR env, then we will not override the env.

strawgate · 2024-08-29T15:21:37Z

For the first part of my question though -- It seems like STORAGE_DIR is probably too general to be using as an environment variable here?

VihasMakwana · 2024-08-29T15:22:54Z

It seems like STORAGE_DIR is probably too general to be using as an environment variable here?

Sorry, I forgot to add it in above comment.

I agree with you. It's too general. I believe we should rename it in context of agent and otel.

strawgate · 2024-08-29T15:23:43Z

I only override STORAGE_DIR if it's not set by the user (by default I use elastic-agent/data/registry/otelcol, but we can change the default).

So doesn't this mean that we no longer need to define this directory in the onboarding yaml? Do we want to create this folder if it doesn't exist?

VihasMakwana · 2024-08-29T15:28:20Z

So doesn't this mean that we no longer need to define this directory in the onboarding yaml? Do we want to create this folder if it doesn't exist?

@strawgate In the onboarding yaml, we use the env variable STORAGE_DIR (quite a general name though). We don't specify any directory explicitly.
Example,

elastic-agent/internal/pkg/otel/samples/linux/logs_metrics_traces.yml

Lines 50 to 52 in fd477ec

    
           extensions: 
        
             file_storage: 
        
               directory: ${env:STORAGE_DIR}

Do we want to create this folder if it doesn't exist?

Yes. I've added that step in this PR.
The OTeL collector requires that the directory should exist or else it will not start up and will throw an error

strawgate · 2024-08-29T15:30:36Z

@strawgate In the onboarding yaml, we use the env variable STORAGE_DIR (quite a general name though). We don't specify any directory explicitly.

This is no longer necessary though after this PR though because if it's not set, it defaults to env:storage_dir, so explicitly setting it to env:storage_dir is redundant and is not necessary anymore?

The onboarding flow uses this yaml and replaces the environment variables with actual values -- so it actually replaces env:STORAGE_DIR with ./data/otelcol which also shouldn't be needed anymore?

VihasMakwana · 2024-08-29T15:42:30Z

This is no longer necessary though after this PR though because if it's not set, it defaults to env:storage_dir, so explicitly setting it to env:storage_dir is redundant and is not necessary anymore?

I believe it's necessary to set it in the onboarding YAML.

If you review the filestorage extension documentation, it specifies that the default directory used is /var/lib/otelcol/file_storage.

If we omit the env:storage_dir in onboarding.yaml, the OpenTelemetry Collector will default to storing offsets in /var/lib/otelcol/file_storage, as per the documentation. This may not be the intended behavior if we are expecting it to use env:storage_dir. Therefore, it's important to set it explicitly to avoid this issue.

Let me give an example to make this clear (hopefully ;)),

if a user has set storage_dir to /path/data/otelcol (because he wants to store offsets in this directory) and he runs the elastic-agent with the otel mode, then this PR will enusre that the /path/data/otelcol directory exists before booting up the otel collector.

However, if we remove the env:storage_dir setting from the onboarding YAML, the otel collector will revert to its default behavior and use the /var/lib/otelcol/file_storage directory for storage.

Therefore, omitting that setting means that the otel will not use the user-specified directory (/path/data/otelcol) and will instead use the default path, which may not align with the user's intended configuration.

Does that make sense?

VihasMakwana · 2024-08-29T15:43:22Z

That being said,

If the OpenTelemetry Collector used env:storage_dir by default, your point would be valid.

VihasMakwana · 2024-08-29T15:49:38Z

@strawgate your understanding is correct.
But we also need to explicitly set env:storage_dir here

elastic-agent/internal/pkg/otel/samples/linux/logs_metrics_traces.yml

Lines 50 to 52 in fd477ec

    
           extensions: 
        
             file_storage: 
        
               directory: ${env:STORAGE_DIR}

strawgate · 2024-08-29T15:49:47Z

I see, so the remaining issue (from my perspective) is just us using the storage_dir environment variable -- though it probably makes more sense for the directory creation to be handled by the extension as in most actual cases this environment var will be unused anyway (as the onboarding flow removes it and a user is unlikely to populate this themselves).

VihasMakwana · 2024-08-29T15:51:59Z

so the remaining issue (from my perspective) is just us using the storage_dir environment variable

yes.

though it probably makes more sense for the directory creation to be handled by the extension as in most cases this environment var will be unused anyway

I agree. But as of now, the extension doesn't handle that part.
We can discuss this with upstream by opening an issue.

VihasMakwana · 2024-08-29T15:55:18Z

@strawgate regarding the name, I believe we should switch to OTEL_STORAGE_DIR or OTEL_REGISTRY_DIR for the env variable. Let me know if you have any suggestions.

strawgate · 2024-08-29T16:03:10Z

extensions:
file_storage:
directory

otel_ext_file_storage_dir?

pchila

I see that we are ensuring the existence of a storage directory here but I am not sure that it's enough to enable persistence by default, without some default receiver/exporter/extensions/service configuration being injected.

Obviously the choice depends on what kind of compatibility we want to keep with upstream OTel collector we want to keep but in my view enabling persistence by default means that I can omit storage extensions settings in my config file and then still benefit from persistence by default but maybe I misunderstood the purpose of the change

pchila · 2024-08-30T08:41:03Z

internal/pkg/otel/run.go

@@ -80,3 +86,23 @@ func newSettings(version string, configPaths []string) (*otelcol.CollectorSettin
 		DisableGracefulShutdown: true,
 	}, nil
 }
+
+func ensureRegistryExists() error {
+	storageDir := os.Getenv("STORAGE_DIR")


nit: reference or define a constant for the env variable name STORAGE_DIR

I also think this should have a more specific name, we should also discuss if we should just reuse the existing STATE_PATH variable that controls the same thing.

pchila · 2024-08-30T09:30:27Z

internal/pkg/otel/run.go

+	if _, err := os.Stat(storageDir); err == nil {
+		// directory exists
+		return nil
+	} else if os.IsNotExist(err) {
+		return os.MkdirAll(storageDir, 0755)
+	} else {
+		return fmt.Errorf("error stating %s: %w", storageDir, err)
+	}


We can maybe restructure this block a bit

Suggested change

if _, err := os.Stat(storageDir); err == nil {

// directory exists

return nil

} else if os.IsNotExist(err) {

return os.MkdirAll(storageDir, 0755)

} else {

return fmt.Errorf("error stating %s: %w", storageDir, err)

}

err := os.Stat(storageDir);

if errors.Is(err, fs.ErrNotExist) {

// create directory if it doesn't exist

return os.MkdirAll(storageDir, 0755)

}

if err != nil {

// we have a generic error

return fmt.Errorf("error stating %s: %w", storageDir, err)

}

return nil

pchila · 2024-08-30T09:41:46Z

internal/pkg/otel/run.go

+	if err := ensureRegistryExists(); err != nil {
+		return fmt.Errorf("error while creating registry: %w", err)
+	}
+


Shouldn't we check and inject the storage extension settings here? It may be conditional to a cli flag or some env variable or something else but I doubt very man people are gonna run the elastic otel collector with our sample config file and without that this change ensures only that a given dir exists/is created...

andrzej-stencel · 2024-08-30T11:48:33Z

This pull request is named "enable persistence by default", but it does not do what the description says.

I believe what this change is intending to achieve is "make sure the directory used by the File Storage extension exists", but I it does not actually do that either.

What this change actually does is:

look at an arbitrary env var STORAGE_DIR (regardless of whether the env var is used in the collector's config or not),
default this env var to a specific path if unset,
create a directory at that path if it does not exist.

I see the following problems with this approach:

It creates a directory in an arbitrary location, regardless of whether this location is being used by the collector for persistence. This may be surprising and cause unwanted side effects for users that don't use the STORAGE_DIR env var in their File Storage extension configuration.
The collector fails to start if that arbitrary directory does not exist and cannot be created. This may be surprising to users that don't use File Storage extension and are running e.g. in a container with read-only file system.

A much better way to achieve the intention "make sure the directory used by the File Storage extension exists" would be to add an option to the upstream File Storage extension like create_directory, which, when set to true, would create the directory that is actually used by the extension, for each extension that is defined (and create no directory if no extension is defined). I suggest to create an issue in Contrib about it.

blakerouse · 2024-08-30T15:25:22Z

Another option is to also ensure that the STORAGE_DIR is created during installation of the Elastic Agent inside of the Elastic Agent directory structure. Use that path as the default and if the user changes the default then its up to the user to create that directory. This would ensure that the path would also have the correct permissions and ownership when being installed.

It would also be nice to add ability to adjust the default in upstream instead of it being hard-coded.

VihasMakwana · 2024-08-30T16:04:53Z

Thank you for your feedback!

I agree that handling directory creation should ideally be managed upstream. I've submitted an enhancement request to address this.

In the meantime, my current PR aimed to ensure the storage directory exists by using the storage_dir environment variable, as it's featured in our sample configs. However, this approach may lead to issues if the user doesn't include this setting in their otel config.

I propose repurposing the PR to focus solely on enabling filestorage in our sample configs. I will then submit a separate PR to address the directory creation flag.

mergify · 2024-09-03T21:19:45Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b enable-persistence-elastic upstream/enable-persistence-elastic
git merge upstream/main
git push upstream enable-persistence-elastic

cmacknz · 2024-09-03T21:36:37Z

internal/pkg/otel/run.go

+	if storageDir == "" {
+
+		// by default use "${path.data}/registry/otelcol" to store offsets
+		storageDir = filepath.Join(paths.Data(), "registry", "otelcol")


I think we probably need some discussion on what our directory structure should be, considering https://github.com/elastic/ingest-dev/issues/3671 and #5304 for example.

There are also things besides filelog that may want disk persistent, who don't use a registry concept. If we needed more than this, where would they go in the directory structure? Here? What if we decided we needed more than one storage extension?

cmacknz · 2024-09-03T23:35:07Z

Taking a higher level look at this, I don't think we need to introduce the .go changes or the env var quite yet. These are not upstream functionality and if we expose them to users they will be hard to get rid of.

Fundamentally, lack of persistence for log collection is sub-optimal but not fatal. As long as we are only considering our pure OTel collector distribution (i.e. not Beats receivers yet) we should solve this with documentation and reference examples while making or advocating for upstream changes to make the configuration changes needed simpler.

+1 to the suggestion in #5347 (comment) to add a create_directory option to the storage extension, and as discussed elsewhere we should try to make it so that receivers that benefit from storage use it automatically if it is available.

For Beats receivers, we will have to store the Filebeat registry somewhere to maintain parity with what we do today, but we already have a place to put that we could reuse without adding anything new. We may opt to change where this state is stored, but until we have to do that let's avoid introducing any new configuration or env vars unless there is a really strong reason. Adding configuration is easy, it is much harder to take away later so let's not do it until we have to and I don't think we have to yet.

mergify · 2024-09-10T21:11:26Z

backport-v8.x has been added to help with the transition to the new branch 8.x.

mergify · 2024-09-11T11:49:42Z

backport-v8.x has been added to help with the transition to the new branch 8.x.

VihasMakwana · 2024-09-11T12:06:52Z

Closing this PR.
Once open-telemetry/opentelemetry-collector-contrib#34985 gets merged, I'll update the docs and raise a new PR as per #5347 (comment)

chore: enable persistence by default in our config files

0c90c0f

VihasMakwana added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Aug 23, 2024

VihasMakwana self-assigned this Aug 23, 2024

mergify bot added the backport-skip label Aug 23, 2024

fix: add fragement

3048a0a

VihasMakwana changed the title ~~chore: enable persistence by default in our OTeL distrubtion~~ [Otel]: enable persistence by default in our OTeL distrubtion Aug 23, 2024

pierrehilbert reviewed Aug 23, 2024

View reviewed changes

changelog/fragments/1724406973-enable-persistence-by-default.yaml Outdated Show resolved Hide resolved

fix: fragement

97036a5

VihasMakwana marked this pull request as ready for review August 23, 2024 10:01

VihasMakwana requested a review from a team as a code owner August 23, 2024 10:01

VihasMakwana requested review from andrzej-stencel and pchila August 23, 2024 10:01

VihasMakwana requested a review from pierrehilbert August 23, 2024 10:04

VihasMakwana removed the backport-skip label Aug 23, 2024

mergify bot added the backport-skip label Aug 23, 2024

VihasMakwana and others added 2 commits August 23, 2024 16:46

Merge branch 'main' into enable-persistence-elastic

21d98e1

fix: ensure registry exists

f32e874

michalpristas approved these changes Aug 29, 2024

View reviewed changes

strawgate changed the title ~~[Otel]: enable persistence by default in our OTeL distrubtion~~ [OTel]: enable persistence by default in our OTeL distribution Aug 29, 2024

strawgate changed the title ~~[OTel]: enable persistence by default in our OTeL distribution~~ [OTel]: enable persistence by default in our OTel distribution Aug 29, 2024

VihasMakwana mentioned this pull request Aug 29, 2024

[testing/integration] - Add integration test cases for OTeL persistence. #5387

Open

pchila reviewed Aug 30, 2024

View reviewed changes

cmacknz reviewed Sep 3, 2024

View reviewed changes

mergify bot added the backport-v8.x label Sep 10, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 11, 2024

VihasMakwana closed this Sep 11, 2024

ycombinator deleted the enable-persistence-elastic branch September 11, 2024 12:08

[OTel]: enable persistence by default in our OTel distribution #5347

[OTel]: enable persistence by default in our OTel distribution #5347

Conversation

VihasMakwana commented Aug 23, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

Disruptive User Impact

How to test this PR locally

Related issues

Questions to ask yourself

mergify bot commented Aug 23, 2024

elasticmachine commented Aug 23, 2024

mergify bot commented Aug 23, 2024

elastic-sonarqube bot commented Aug 23, 2024

Quality Gate passed

VihasMakwana commented Aug 29, 2024

strawgate commented Aug 29, 2024

ycombinator commented Aug 29, 2024

VihasMakwana commented Aug 29, 2024 • edited Loading

strawgate commented Aug 29, 2024 • edited Loading

VihasMakwana commented Aug 29, 2024

strawgate commented Aug 29, 2024 • edited Loading

VihasMakwana commented Aug 29, 2024

strawgate commented Aug 29, 2024

VihasMakwana commented Aug 29, 2024

strawgate commented Aug 29, 2024 • edited Loading

VihasMakwana commented Aug 29, 2024

VihasMakwana commented Aug 29, 2024

VihasMakwana commented Aug 29, 2024

strawgate commented Aug 29, 2024 • edited Loading

VihasMakwana commented Aug 29, 2024

VihasMakwana commented Aug 29, 2024 • edited Loading

strawgate commented Aug 29, 2024 • edited Loading

pchila left a comment

Choose a reason for hiding this comment

pchila Aug 30, 2024

Choose a reason for hiding this comment

cmacknz Sep 3, 2024

Choose a reason for hiding this comment

pchila Aug 30, 2024

Choose a reason for hiding this comment

pchila Aug 30, 2024

Choose a reason for hiding this comment

andrzej-stencel commented Aug 30, 2024

blakerouse commented Aug 30, 2024

VihasMakwana commented Aug 30, 2024

mergify bot commented Sep 3, 2024

cmacknz Sep 3, 2024

Choose a reason for hiding this comment

cmacknz commented Sep 3, 2024 • edited Loading

mergify bot commented Sep 10, 2024

mergify bot commented Sep 11, 2024

VihasMakwana commented Sep 11, 2024

VihasMakwana commented Aug 23, 2024 •

edited

Loading

VihasMakwana commented Aug 29, 2024 •

edited

Loading

strawgate commented Aug 29, 2024 •

edited

Loading

strawgate commented Aug 29, 2024 •

edited

Loading

strawgate commented Aug 29, 2024 •

edited

Loading

strawgate commented Aug 29, 2024 •

edited

Loading

VihasMakwana commented Aug 29, 2024 •

edited

Loading

strawgate commented Aug 29, 2024 •

edited

Loading

cmacknz commented Sep 3, 2024 •

edited

Loading