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

Fix profiling naming issues #27133

Merged
merged 1 commit into from
Nov 6, 2017
Merged

Fix profiling naming issues #27133

merged 1 commit into from
Nov 6, 2017

Conversation

polyfractal
Copy link
Contributor

Some code-paths use anonymous classes (such as NonCollectingAggregator in terms agg), which messes up the display name of the profiler. If we encounter an anonymous class, we need to grab the super's name.

Another naming issue was that ProfileAggs were not delegating to the wrapped agg's name for toString(). That meant some places were showing the class name instead of the user-defined name.

This PR also fixes up the profile documentation. Some of the examples were executing against empty indices, which shows different profile results than a populated index (and made for confusing examples).

Finally, I switched the agg display names from the fully qualified name to the simple name, so that it's similar to how the query profiles work.

I don't think any of this is BWC breaking since it's just changing the display name from useless/ugly to useful... but happy to hear alternative thoughts. :)

@polyfractal
Fix profiling naming issues
553c24c 
Some code-paths use anonymous classes (such as NonCollectingAggregator
in terms agg), which messes up the display name of the profiler.  If
we encounter an anonymous class, we need to grab the super's name.

Another naming issue was that ProfileAggs were not delegating to the
wrapped agg's name for toString(), leading to ugly display.

This PR also fixes up the profile documentation.  Some of the examples were
executing against empty indices, which shows different profile results
than a populated index (and made for confusing examples).

Finally, I switched the agg display names from the fully qualified name
to the simple name, so that it's similar to how the query profiles work.

Closes elastic#26405
@polyfractal polyfractal added :Search/Search Search-related issues that do not fall into other categories review labels Oct 26, 2017
@polyfractal polyfractal removed the review label Nov 6, 2017
@polyfractal polyfractal merged commit 6e9e07d into elastic:master Nov 6, 2017
polyfractal added a commit that referenced this pull request Nov 6, 2017
@polyfractal
Fix profiling naming issues (#27133)
b15484f 
Some code-paths use anonymous classes (such as NonCollectingAggregator
in terms agg), which messes up the display name of the profiler.  If
we encounter an anonymous class, we need to grab the super's name.

Another naming issue was that ProfileAggs were not delegating to the
wrapped agg's name for toString(), leading to ugly display.

This PR also fixes up the profile documentation.  Some of the examples were
executing against empty indices, which shows different profile results
than a populated index (and made for confusing examples).

Finally, I switched the agg display names from the fully qualified name
to the simple name, so that it's similar to how the query profiles work.

Closes #26405
jasontedor added a commit that referenced this pull request Nov 7, 2017
@jasontedor
Merge branch 'master' into ccr
3b6d0ce 
* master: (25 commits)
  Disable bwc tests in preparation of backporting #26931
  TemplateUpgradeService should only run on the master (#27294)
  Die with dignity while merging
  Fix profiling naming issues (#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (#27283)
  Add an active Elasticsearch WordPress plugin link (#27279)
  Setting url parts as required to reflect the code base (#27263)
  keys in aggs percentiles need to be in quotes. (#26905)
  Align routing param type with search.json (#26958)
  Update to support bulk updates by query (#27172)
  Remove duplicated SnapshotStatus (#27276)
  add split index reference in indices.asciidoc
  Add ability to split shards (#26931)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535)
  Upgrade to Jackson 2.8.10 (#27230)
  Fix inconsistencies in the rest api specs for `tasks` (#27163)
  Adjust RestHighLevelClient method modifiers (#27238)
  Remove unused parameters in AnalysisRegistry (#27232)
  Add more information on `_failed_to_convert_` exception (#27034)
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Nov 7, 2017
@jasontedor
Merge branch 'ccr' into pr/27094
13d373b 
* ccr: (127 commits)
  Disable bwc tests in preparation of backporting elastic#26931
  TemplateUpgradeService should only run on the master (elastic#27294)
  Die with dignity while merging
  Fix profiling naming issues (elastic#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (elastic#27283)
  Add an active Elasticsearch WordPress plugin link (elastic#27279)
  Setting url parts as required to reflect the code base (elastic#27263)
  keys in aggs percentiles need to be in quotes. (elastic#26905)
  Align routing param type with search.json (elastic#26958)
  Update to support bulk updates by query (elastic#27172)
  Remove duplicated SnapshotStatus (elastic#27276)
  add split index reference in indices.asciidoc
  Add ability to split shards (elastic#26931)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (elastic#25535)
  Upgrade to Jackson 2.8.10 (elastic#27230)
  Fix inconsistencies in the rest api specs for `tasks` (elastic#27163)
  Adjust RestHighLevelClient method modifiers (elastic#27238)
  Remove unused parameters in AnalysisRegistry (elastic#27232)
  Add more information on `_failed_to_convert_` exception (elastic#27034)
  ...
jasontedor added a commit that referenced this pull request Nov 7, 2017
@jasontedor
Merge branch '6.x' into ccr-6.x
efe0981 
* 6.x:
  Update shrink's bwc version to 6.1.0
  add split index reference in indices.asciidoc
  Add ability to split shards (#26931)
  TemplateUpgradeService should only run on the master (#27294)
  Die with dignity while merging
  Fix profiling naming issues (#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (#27283)
  Add an active Elasticsearch WordPress plugin link (#27279)
  Setting url parts as required to reflect the code base (#27263)
  keys in aggs percentiles need to be in quotes. (#26905)
  Align routing param type with search.json (#26958)
  Update to support bulk updates by query (#27172)
  Remove duplicated SnapshotStatus (#27276)
jasontedor added a commit to glefloch/elasticsearch that referenced this pull request Nov 9, 2017
@jasontedor
Merge branch 'master' into pr/25003
2d36d41 
* master: (556 commits)
  Fix find remote when building BWC
  Remove colons from task and configuration names
  Add unreleased 5.6.5 version number
  testCreateSplitIndexToN: do not set `routing_partition_size` to >= `number_of_routing_shards`
  Snapshot/Restore: better handle incorrect chunk_size settings in FS repo (elastic#26844)
  Add limits for ngram and shingle settings (elastic#27211) (elastic#27318)
  Correct comment in index shard test
  Roll translog generation on primary promotion
  ObjectParser: Replace IllegalStateException with ParsingException (elastic#27302)
  scripted_metric _agg parameter disappears if params are provided (elastic#27159)
  Update discovery-ec2.asciidoc
  Update shrink's bwc version to 6.1.0 and enabled bwc tests
  Add limits for ngram and shingle settings (elastic#27211)
  Disable bwc tests in preparation of backporting elastic#26931
  TemplateUpgradeService should only run on the master (elastic#27294)
  Die with dignity while merging
  Fix profiling naming issues (elastic#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (elastic#27283)
  Add an active Elasticsearch WordPress plugin link (elastic#27279)
  ...
@jasontedor
Copy link
Member

@polyfractal This PR has the "backport pending" label but it appears to have been backported to all of the versions with labels on this PR? Can the label be removed now?

@polyfractal
Copy link
Contributor Author

@jasontedor I was thinking this should go into 6.0.1, not sure why I didn't add the 6.0.1 label though (probably just overlooked it). I'll add the label

@jasontedor
Copy link
Member

Thanks, and this can be safely backported now that 6.0 is GA and then we can remove the backport pending label here.

@polyfractal
Copy link
Contributor Author

polyfractal commented Nov 20, 2017 •
edited
Loading

Backported in 5990364, removed label. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v6.0.1 v6.1.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@polyfractal @jasontedor @jpountz @clintongormley @colings86