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.

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

Jump to bottom

feat: migrate metadata decoding to protobuf #67

Merged
merged 7 commits into from
Jun 19, 2023
Merged

feat: migrate metadata decoding to protobuf #67

merged 7 commits into from
Jun 19, 2023

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented Jun 18, 2023 •
edited
Loading

This should be the last big PR related to protobuf.

Migrate metadata decoding to protobuf and update the code.
Fix a big in rum decoding setting the wrong timestamp
Disable reflection tests again, they are currently broken. (we really should move away from this)

Related to #52

@kruskall kruskall requested a review from a team June 18, 2023 23:12
@kruskall kruskall mentioned this pull request Jun 18, 2023
Merged
@axw
Copy link
Member

axw commented Jun 19, 2023

Disable reflection tests again, they are currently broken. (we really should move away from this)

Are they disabled? It doesn't look like it. Did you revert a change, and forget to update the description?
(I don't think we should disable them until we have replacement coverage.)

I just ran the tests locally and got this panic:

=== RUN   TestConcurrentAsync/semaphore_empty                                                                                                                                                                                                                                     
panic: assignment to entry in nil map                                                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                                  
goroutine 180 [running]:                                                                                                                                                                                                                                                          
github.com/elastic/apm-data/model/modelpb.Labels.Set(...)                                                                                                                                                                                                                         
        /home/andrew/projects/apm-data/model/modelpb/labels.go:27                                                                        
github.com/elastic/apm-data/input/elasticapm/internal/modeldecoder/modeldecoderutil.MergeLabels(0x0?, 0xc000170f00)                      
        /home/andrew/projects/apm-data/input/elasticapm/internal/modeldecoder/modeldecoderutil/labels.go:57 +0x205                       
github.com/elastic/apm-data/input/elasticapm/internal/modeldecoder/modeldecoderutil.GlobalLabelsFrom(0xffffffffffffffff?, 0xc000170f00)  
        /home/andrew/projects/apm-data/input/elasticapm/internal/modeldecoder/modeldecoderutil/labels.go:32 +0xa5                        
github.com/elastic/apm-data/input/elasticapm/internal/modeldecoder/v2.mapToMetadataModel(0xc00026c480, 0xc000170f00)                     
        /home/andrew/projects/apm-data/input/elasticapm/internal/modeldecoder/v2/decoder.go:576 +0x3a9                                   
github.com/elastic/apm-data/input/elasticapm/internal/modeldecoder/v2.decodeMetadata(0xe3ac00, {0xf0a220, 0xc00064e010}, 0xc00014e000?)  
        /home/andrew/projects/apm-data/input/elasticapm/internal/modeldecoder/v2/decoder.go:279 +0x193                                                                                                                                                                            
github.com/elastic/apm-data/input/elasticapm/internal/modeldecoder/v2.DecodeNestedMetadata(...)                                                                                                                                                                                   
        /home/andrew/projects/apm-data/input/elasticapm/internal/modeldecoder/v2/decoder.go:166                                          
github.com/elastic/apm-data/input/elasticapm.(*Processor).readMetadata(0xc000726070?, 0xc00064e010, 0xc00014e000?)                       
        /home/andrew/projects/apm-data/input/elasticapm/processor.go:126 +0x1d5                                                          
github.com/elastic/apm-data/input/elasticapm.(*Processor).HandleStream(0xc000726070, {0xf12c68, 0xc00041e090}, 0x1, 0xc000780000?, {0xf0c600, 0xc00014e000}, 0xc23d4c?, {0xf0a200, 0xc000322120}, ...)                                                                            
        /home/andrew/projects/apm-data/input/elasticapm/processor.go:274 +0x13a                                                          
github.com/elastic/apm-data/input/elasticapm.TestConcurrentAsync.func1.1.1()                                                             
        /home/andrew/projects/apm-data/input/elasticapm/processor_test.go:396 +0x1a5                                                     
created by github.com/elastic/apm-data/input/elasticapm.TestConcurrentAsync.func1.1                                                      
        /home/andrew/projects/apm-data/input/elasticapm/processor_test.go:393 +0x18c                                                     
FAIL    github.com/elastic/apm-data/input/elasticapm    0.225s

@kruskall
Copy link
Member Author

Are they disabled? It doesn't look like it. Did you revert a change, and forget to update the description?
(I don't think we should disable them until we have replacement coverage.)

They are currently disabled (see https://github.com/elastic/apm-data/pull/67/files#diff-ce8c691488600b0f16281dec1b98c19dabd7edaa32cdc19df046a5829d23d9c4R93). I tried to fix them but found myself rewriting a large portion of the tests.
I don't like disabling them but I don't think they can be easily updated.

I just ran the tests locally and got this panic:

check notes

That's... not supposed to happen. I'll look into it

@axw
Copy link
Member

axw commented Jun 19, 2023

I just ran the tests locally and got this panic:

check notes

That's... not supposed to happen. I'll look into it

It seems to be a data race -- try running with -race.

@kruskall
test: fix data race in TestConcurrentAsync
18b0ebc 
We were previously using a struct so this wasn't obvious. Now that we
are passing a pointer we can't share the modelpb.APMEvent because the
readMetadata is called before cloning the base event.
The data race only happens in the test because of how it's written.
Production code (APM Server) is not sharing APMEvents.
@kruskall
Copy link
Member Author

Should be fixed!

@axw
Copy link
Member

axw commented Jun 19, 2023

@kruskall can you please add -race to the make test target, so this kind of thing gets caught in CI?

Just having a look at the skipped reflection code now.

@axw
Copy link
Member

axw commented Jun 19, 2023

@kruskall the skips can be removed by cherry-picking 436bc19

@kruskall
Copy link
Member Author

kruskall commented Jun 19, 2023 •
edited
Loading

@axw Thank you! 🙇

Some unrelated (semaphore) tests are hanging when run with the race detector. I've opened a followup issue: #68

@kruskall kruskall merged commit 3ff5775 into elastic:main Jun 19, 2023
@kruskall kruskall deleted the feat/migrate-metadata-pb branch June 19, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axw axw axw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kruskall @axw