Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

Use service name as debuggee id on gke#275

Merged
matthewloring merged 1 commit intogoogleapis:masterfrom
matthewloring:gke-id
Jun 12, 2017
Merged

Use service name as debuggee id on gke#275
matthewloring merged 1 commit intogoogleapis:masterfrom
matthewloring:gke-id

Conversation

@matthewloring
Copy link
Copy Markdown
Contributor

@matthewloring matthewloring commented Jun 8, 2017

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 8, 2017
ofrobots
ofrobots suggested changes Jun 9, 2017
View reviewed changes
src/agent/debuglet.js Outdated
return callback(null, project, onGCP);
metadata.instance(
'attributes/cluster-name', function(err, response, metadataCluster) {
return callback(null, project, metadataCluster, onGCP);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/agent/debuglet.js Outdated
Debuglet.createDebuggee =
function(projectId, uid, serviceContext, sourceContext, description,
errorMessage, onGCP) {
errorMessage, onGCP, clusterName) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@matthewloring
Copy link
Copy Markdown
Contributor Author

matthewloring commented Jun 12, 2017

PTAL.

@matthewloring matthewloring merged commit d3994f8 into googleapis:master Jun 12, 2017
@matthewloring matthewloring deleted the gke-id branch June 12, 2017 16:10
@bmenasha
Copy link
Copy Markdown

bmenasha commented Jun 18, 2017

This change broke the debugee ID on App Engine Flexible (and probably raw GCE as well).

There won't be a cluster-name attribute for non GKE clusters and the code ignores the "err" from the instance metadata get which causes the 404 error message to be supplied as the module name see:

d3994f8#diff-c22bef9d940a3ddb15ce23102dac9da6R354

metadata.instance(
'attributes/cluster-name', function(err, response, metadataCluster) {
return callback(null,
{ project: project, clusterName: metadataCluster, onGCP: onGCP});

This will then create a debugger target with this 404 error message embedded right in the description as well as losing the "version" information:

{
  "id": "gcp:900559217049:4727c82bc57d90e6",
  "project": "<redacted>",
  "uniquifier": "d827a6ac3c4f835841f3f5e87254f1bbe76a11dd",
  "description": "node app.js module:\u003c!DOCTYPE html\u003e\n\u003chtml lang=en\u003e\n  \u003cmeta charset=utf-8\u003e\n  \u003cmeta name=viewport content=\"initial-scale=1, minimum-scale=1, width=device-width\"\u003e\n  \u003ctitle\u003eError 404 (Not Found)!!1\u003c/title\u003e\n  \u003cstyle\u003e\n    *{margin:0;padding:0}html,code{font:15px/22px arial,sans-serif}html{background:#fff;color:#222;padding:15px}body{margin:7% auto 0;max-width:390px;min-height:180px;padding:30px 0 15px}* \u003e body{background:url(//www.google.com/images/errors/robot.png) 100% 5px no-repeat;padding-right:205px}p{margin:11px 0 22px;overflow:hidden}ins{color:#777;text-decoration:none}a img{border:0}@media screen and (max-width:772px){body{background:none;margin-top:0;max-width:none;padding-right:0}}#logo{background:url(//www.google.com/images/branding/googlelogo/1x/googlelogo_color_150x54dp.png) no-repeat;margin-left:-5px}@media only screen and (min-resolution:192dpi){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat 0% 0%/100% 100%;-moz-border-image:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) 0}}@media only screen and (-webkit-min-device-pixel-ratio:2){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat;-webkit-background-size:100% 100%}}#logo{display:inline-block;height:54px;width:150px}\n  \u003c/style\u003e\n  \u003ca href=//www.google.com/\u003e\u003cspan id=logo aria-label=Google\u003e\u003c/span\u003e\u003c/a\u003e\n  \u003cp\u003e\u003cb\u003e404.\u003c/b\u003e \u003cins\u003eThat’s an error.\u003c/ins\u003e\n  \u003cp\u003eThe requested URL \u003ccode\u003e/computeMetadata/v1/instance/attributes/cluster-name\u003c/code\u003e was not found on this server.  \u003cins\u003eThat’s all we know.\u003c/ins\u003e\n version:unversioned",
  "agentVersion": "google.com/node-gcp/v2.1.0",
  "sourceContexts": [
    {
      "cloudRepo": {
        "repoId": {
          "projectRepoId": {
            "projectId": "<redacted>",
            "repoName": "<redacted>"
          }
        },
        "revisionId": "9e5598185aa2ae4ec3ff69ac3b3a7486480c1997"
      }
    }

Thanks for correcting.
Ben

ofrobots added a commit to ofrobots/cloud-debug-nodejs that referenced this pull request Jun 19, 2017
@ofrobots
Revert "Use service name as debuggee id on gke (googleapis#275)"
5d281c3 
This reverts commit d3994f8 as it
breaks on non-GKE GCP enviornments.

See: googleapis#275 (comment)
@ofrobots ofrobots mentioned this pull request Jun 19, 2017
Merged
ofrobots added a commit that referenced this pull request Jun 19, 2017
@ofrobots
Revert "Use service name as debuggee id on gke (#275)" (#278)
66d67f8 
This reverts commit d3994f8 as it
breaks on non-GKE GCP enviornments.

See: #275 (comment)
@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Jun 19, 2017

Thanks for reporting. Reverted in #278.

@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Jun 19, 2017

The fix has been released in version 2.1.1.

DominicKramer added a commit to DominicKramer/cloud-debug-nodejs that referenced this pull request Jun 23, 2017
@DominicKramer
Squashed commit of the following:
20296b4 
commit 69106322b587bc6152199cdc0ec9b70f47afba03
Author: Dominic Kramer <[email protected]>
Date:   Mon Jun 19 13:51:32 2017 -0700

    Convert debuglet.ts to use Typescript classes

    The conversion now includes the changes in PR googleapis#278 that reverts
    PR googleapis#275 (Use service name as debuggee id on gke).

commit afa5e7e5a5df2534e9265f01df61aed6d8b11327
Author: Dominic Kramer <[email protected]>
Date:   Mon Jun 19 13:48:46 2017 -0700

    Rename debuglet.js to debuglet.ts
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DominicKramer DominicKramer Awaiting requested review from DominicKramer

1 more reviewer

@ofrobots ofrobots ofrobots approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matthewloring @bmenasha @ofrobots @jmdobry @googlebot