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

Split object_space_dump into smaller messages #77

Merged
merged 15 commits into from
Jan 24, 2015
Merged

Split object_space_dump into smaller messages #77

merged 15 commits into from
Jan 24, 2015

Conversation

emilsoman
Copy link
Contributor

Closes #52

Merge after #76 is merged.

Stuff that changed :

  1. Added snapshot_no to obj space dump events (so these can be grouped even if they arrive in different messages)
  2. Accumulated events will be sent prematurely(before the 1 second gap) if we have more than a fixed number of events in the queue (set to 400 for now after some experimentation)
  3. Added a limit of 20 object dumps to be grouped into one event. So we'll at a time receive a max of 20*400 object dumps in one zmq message.

@gnufied
Copy link
Contributor

gnufied commented Nov 1, 2014

@emilsoman can you document Message format after this change? Potentially in a new WIKI page so as not to remove existing one.

@emilsoman
Copy link
Contributor Author

@gnufied , the only change is a new key that's added to the payload of each object space dump message called "snapshot_no" , I've added this to the wiki already

@gnufied
Copy link
Contributor

gnufied commented Nov 5, 2014

@emilsoman can you update this PR so as it is mergeable?

@emilsoman
Copy link
Contributor Author

@gnufied , done.

// messages. This macro defines the number
// of object data that should be packed
// as the payload of one message.
#define MAX_OBJECT_DUMPS_IN_MESSAGE 20
Copy link
Contributor

Choose a reason for hiding this comment

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

20 is very small number emil and I think for larger projects we will see drop in messages, because queue will be too full.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this number has to be way higher, but it has to be calibrated. But something like 1000 should not be too bad of a limit IMO.

@gnufied
Copy link
Contributor

gnufied commented Jan 20, 2015

@emilsoman can we update this PR with latest master?!

@gnufied gnufied added this to the v0.3.0 CPU profiling milestone Jan 22, 2015
This version of protocol is not compatible with
previous version.
@gnufied
Copy link
Contributor

gnufied commented Jan 23, 2015

Okay, so this looks mostly good. One problem though is - this specifically implements splitting of heap dumps which is fine.

But I think, we will require similar splitting of larger cpu profiling dumps, so it makes sense to make it bit generic.

I think it makes sense to add a new field called correlation-id to all events, for other events the value can be anything (Sometime like Time.now.to_f.to_s), but for events that can be split, they will have same correlation-id . So if a snapshot event is being split across several messages, all those event messages will have same correlation-id.

What I am really proposing is simply renaming snapshot_no to correlation_id, but I think it makes entire thing more simpler.

@gnufied
Copy link
Contributor

gnufied commented Jan 23, 2015

On seconds thoughts, adding correlation-id to all events may not be necessary, but just to events which can be split is fine!

So really, my only request is to just rename the field itself. :-)

gnufied pushed a commit that referenced this pull request Jan 24, 2015
Split object_space_dump into smaller messages
@gnufied gnufied merged commit 525154f into master Jan 24, 2015
@gnufied gnufied deleted the split-dump branch January 24, 2015 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split event collection messages and send smaller sized messages
2 participants